-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix regrid for the radiation module #345
fix regrid for the radiation module #345
Conversation
@wjge Can you update the PelePhysics submodule here to the version from your PelePhysics PR? |
@@ -288,6 +288,16 @@ PeleLM::regrid(int lbase, amrex::Real time, bool initial) | |||
} | |||
|
|||
finest_level = new_finest; | |||
|
|||
#ifdef PELE_USE_RADIATION |
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.
Just a comment: it looks like all the radiation stuff is controlled at both compile time (PELE_USE_RADIATION
) and the runtime (do_rad_solve
). Now that the radiation module is part of PelePhysics, I think we can remove the #ifdefs and just always compile with radiation and use the runtime flag to determine whether the model gets used. That would make CI a bit simpler. @wjge do you agree? If so I can do this in a future PR.
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.
Now the PeleLMeX example 'tests' do not cover any EB case with soot and radiation modules on for compiling. I am running two production runs (one without EB, one with EB) right now to see whether there is any other issue. Of course, the physics and chemistry might cause other issues that are not related to the solvers. I would recommend we have a EB test with soot and radiation modules on first before getting rid of the macro. What do you think?
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.
Despite the lack of test coverage for EB with radiation, removing the macro would allow some parts of the CI like the clang-tidy
check to cover more of the radiation stuff. And if the runtime options default to off, having these compiled in but inactive shouldn't affect any cases being run. It would be great to add EB tests for these capabilities if you have suitable cases though! In any event, this is tangential to the present PR, which appears to be working with the PelePhysics update so I will approve and merge.
Could you tell me the best workflow for that? Change the CI to checkout the PR in PelePhysics? |
This is done as a regular git commit. In your branch of PeleLMeX, go into the |
When the regrid() remakes any levels, the radiation module will fail.