-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
662 more strict tolerance for unitintegration tests #670
662 more strict tolerance for unitintegration tests #670
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
==========================================
- Coverage 93.33% 93.19% -0.15%
==========================================
Files 53 53
Lines 3603 3586 -17
==========================================
- Hits 3363 3342 -21
- Misses 240 244 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! just a couple minor suggestions
@@ -212,8 +212,8 @@ void test_simple_system( | |||
EXPECT_EQ(result.state_, (micm::SolverState::Converged)); | |||
for (std::size_t i = 0; i < NUM_CELLS; ++i) | |||
{ | |||
EXPECT_NEAR(combined_error(k1[i], state.rate_constants_[i][0], 1e-15), 0, relative_tolerance); | |||
EXPECT_NEAR(combined_error(k2[i], state.rate_constants_[i][1], 1e-15), 0, relative_tolerance); | |||
EXPECT_NEAR(k1[i], state.rate_constants_[i][0], relative_tolerance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think absolute difference is used in EXPECT_NEAR
?
@@ -623,7 +605,7 @@ void test_analytical_photolysis( | |||
template<class BuilderPolicy, class StateType = micm::State<>> | |||
void test_analytical_stiff_photolysis( | |||
BuilderPolicy builder, | |||
double tolerance = 1e-6, | |||
double tolerance = 2e-5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to increase the default tolerance here?
test_analytical_photolysis<builderType, stateType>(two, 2e-6, copy_to_device, copy_to_host); | ||
test_analytical_photolysis<builderType, stateType>(three, 2e-6, copy_to_device, copy_to_host); | ||
test_analytical_photolysis<builderType, stateType>(four, 2e-8, copy_to_device, copy_to_host); | ||
test_analytical_photolysis<builderType, stateType>(four_da, 2e-6, copy_to_device, copy_to_host); | ||
test_analytical_photolysis<builderType, stateType>(six_da, 2e-6, copy_to_device, copy_to_host); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looks like the photolysis tests are less accurate now?
@@ -218,19 +218,19 @@ TEST(AnalyticalExamplesCudaRosenbrock, Oregonator) | |||
}; | |||
|
|||
auto solver = rosenbrock_solver(micm::RosenbrockSolverParameters::TwoStageRosenbrockParameters()); | |||
test_analytical_oregonator<builderType1Cell, stateType1Cell>(solver, 1e-3, copy_to_device, copy_to_host); | |||
test_analytical_oregonator<builderType1Cell, stateType1Cell>(solver, 2e-3, copy_to_device, copy_to_host); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also oregonator test is less accurate now. Is this the problematic test you referred to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @K20shores for addressing my comments. They look good to me!
Closes #662
This also corrects the oregonator test translation which happened in #593 (originally introduced in #222). We weren't actually testing anything useful. However, now we have the same shape and order of magnitude. I don't understand why I had to do the transformations on the true values. I do get why I needed to multiple time by the tau transformation value, but not anything else about it