-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
228 hires #229
Conversation
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 questions
include/micm/solver/rosenbrock.hpp
Outdated
@@ -189,7 +193,7 @@ namespace micm | |||
/// @param rate_constants List of rate constants for each needed species | |||
/// @param number_densities The number density of each species | |||
/// @param forcing Vector of forcings for the current conditions | |||
void CalculateForcing( | |||
virtual void CalculateForcing( |
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.
is it necessary to make this virtual? it looks like you are using the extending class directly in the tests
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.
Well, when I didn't have virtual here, the functions I defined weren't called, I'll try removing this now and see if it breaks
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.
Yeah, removing virtual
here broke the tests
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.
do you think it's worth using the set of processes we derived for the Oregonator test instead of extending RosebrockSolver
? I'm just wondering if we want to change the solver class just for a test
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.
That's fine. I'll have to translate the HIRES and E5 equations though.
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.
oh, well maybe it's not worth the change then. we can just keep the virtual functions and always change later on if we want to
include/micm/solver/rosenbrock.hpp
Outdated
@@ -210,7 +214,7 @@ namespace micm | |||
/// @param rate_constants List of rate constants for each needed species | |||
/// @param number_densities The number density of each species | |||
/// @param jacobian The matrix of partial derivatives | |||
void CalculateJacobian( | |||
virtual void CalculateJacobian( |
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.
same as above
Closes #228