Skip to content
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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 21, 2024
Merged

fix regrid for the radiation module #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65a93cc
fix regrid for radiation module
wjge Feb 15, 2024
c9effce
pull the PR477 from the PelePhysics repo
wjge Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Source/PeleLMeX_Regrid.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,16 @@ PeleLM::regrid(int lbase, amrex::Real time, bool initial)
}

finest_level = new_finest;

#ifdef PELE_USE_RADIATION
Copy link
Collaborator

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.

Copy link
Member Author

@wjge wjge Feb 16, 2024 •
edited
Loading

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?

Copy link
Collaborator

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.

if (do_rad_solve) {
#ifdef AMREX_USE_EB
rad_model->initVars(grids, dmap, m_factory);
#else
rad_model->initVars(grids, dmap);
#endif
}
#endif
}
}

Expand Down
2 changes: 1 addition & 1 deletion Submodules/PelePhysics
Open in desktop
Submodule PelePhysics updated 1 files
+33 −20 Source/Radiation/PeleLMRad.H
Loading