Support negative passive scalars #458
Conversation
ChunYen-Chen
left a comment
ChunYen-Chen
left a comment
There was a problem hiding this comment.
@technic960183 Thanks for the contribution. I left some comments.
I am confused by the variable name in this PR.
- Although the global variable is named
PassiveVar_Floor, it does not exclusively store information for passive scalars. For example, the first bit ofPassiveVar_Flooris not the flag of the first passive scalar. Was this design made for future extensibility? - Some functions use an input argument named
FloorVar, but the name does not reflect its purpose. How about renaming it toPassiveFlooror something that explicitly includes the termPassive? - In this PR, both
PassiveVar_FloorandFloorVaract as flags indicating whether to apply a zero floor to the passive scalars. How about includingFlagin their names for clarity? For example,Flag_PassiveFloor. - Currently, we do not support custom floor values for each passive scalar. It might be helpful to clarify in the documentation that
Floorrefers specifically to a zero floor.
test_problem_deprecated/Model_ParticleOnly/TwoParOrbit/Flu_ResetByUser.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
@ChunYen-Chen Thanks for the review.
Although the global variable is named
PassiveVar_Floor, it does not exclusively store information for passive scalars. For example, the first bit ofPassiveVar_Flooris not the flag of the first passive scalar. Was this design made for future extensibility?
I do this just to match the FieldIdx as FixUpVar_Flux and FixUpVar_Restrict. Becuase our fields related data structure is not stored in an object-oriented way, breaking the direct match between the bits in PassiveVar_Floor and FieldIdx will deduce the maintianability, as it will need to assume that the passive fields are placed behind the fluid fields. I know that there are a lot of for loop assuming it, but I still don't want to introduce more. This is the reason that I take this designing approach.
Some functions use an input argument named
FloorVar, but the name does not reflect its purpose. How about renaming it toPassiveFlooror something that explicitly includes the term Passive?
Agree. I'll replace them after we resolve the previous point.
In this PR, both
PassiveVar_FloorandFloorVaract as flags indicating whether to apply a zero floor to the passive scalars. How about including Flag in their names for clarity? For example,Flag_PassiveFloor.
I'm mimicking FixUpVar_Flux and FixUpVar_Restrict. Is there a reason that these two variables are not flags?
Currently, we do not support custom floor values for each passive scalar. It might be helpful to clarify in the documentation that Floor refers specifically to a zero floor.
Agree. I doc it in the comment of AddField().
test_problem_deprecated/Model_ParticleOnly/TwoParOrbit/Flu_ResetByUser.cpp
Show resolved
Hide resolved
ChunYen-Chen
left a comment
ChunYen-Chen
left a comment
There was a problem hiding this comment.
@technic960183 Thanks for the update. The PR looks good to me.
There was a problem hiding this comment.
@hfhsieh Thanks for your review. I have addressed all of your comments.
You may find it easier to check them commit by commit, especially for the part of fixing the parameters and the headers.
Some of the header and function calls which are too long in a single line. But I won't address this issue in this PR. Since I'm preparing another PR to refactor all of the headers at once.
technic960183
left a comment
technic960183
left a comment
There was a problem hiding this comment.
@hfhsieh Thanks for your review.
I'll update to the least main branch after your approval.
hfhsieh
left a comment
hfhsieh
left a comment
There was a problem hiding this comment.
@technic960183 If coding style of function prototype will be addressed in another PR, please feel free to ignore my earlier related comments.
technic960183
left a comment
technic960183
left a comment
There was a problem hiding this comment.
Yes. Nearly all of the function will be refactored soon (~2 month).
We want to prevent chnages of tons of headers again in the futrue.
I'll coordinate all of the developers once it is ready.
hfhsieh
left a comment
hfhsieh
left a comment
There was a problem hiding this comment.
Thanks for your effort. Looks good to me.
with `Null`, `No` and `Yes` three possible values. This only sets up the header, not the implementation.
And implement the check, taking notes and dumping to HDF5.
We search for all of the `FMAX` to locate the places where we need to add the `PassiveVar_Floor` functionality. Still, `Hydro_IsUnphysical` is found by `NCOMP_PASSIVE`. Note that `case UNPHY_MODE_PASSIVE_ONLY:` in `Hydro_IsUnphysical` loop as `for (int v=0; v<NCOMP_PASSIVE; v++)` instead of `for (int v=NCOMP_FLUID; v<NCOMP_TOTAL; v++)`
Fixed by passing the global variable `PassiveVar_Floor` as a parameter to the kernel functions, layer by layer. This include editing: - 6 function bodies - 42 headers & calls - 8 comments And `Hydro_IsUnphysical` has now been fixed yet. Because the number of its references is too large, and I need a seperate commit to fix it. Actually, this leads to PR gamer-project#443. And I rebase this feature branch after the PR is merged.
Includes implementation of `FloorVar` in `Hydro_IsUnphysical`, updating the header and function calls.
There are 5 steps in the comment `Procedure for outputting new variables:` I have only implemented the first and the third before. This commit implements the rest of the steps.
Co-authored-by: He-Feng Hsieh <[email protected]> Thanks for the code review.
Co-authored-by: He-Feng Hsieh <[email protected]>
The global variable `Flag_PassiveFloor` should not been used in the code shared between CPU and GPU. Instead, it should be passed as a parameter `long PassiveFloor`. In this commit: - Replace `Flag_PassiveFloor` with `PassiveFloor` in the shared calls. - Add parameter `long PassiveFloor` to `Hydro_Con2Pri()`. - Update calls to `Hydro_Con2Pri()`. - Remove incorrect header docs about `Flag_PassiveFloor`. Todo: - Update the function which need `PassiveFloor` to be passed to pass it to `Hydro_Con2Pri()`.
Update 3 functions and their function calls: `CPU_dtSolver_HydroCFL` `CUFLU_dtSolver_HydroCFL` `CUAPI_Asyn_dtSolver`
Update 7 functions and their function calls: `Hydro_RiemannSolver_Exact` `Hydro_RiemannSolver_HLLC` `Hydro_RiemannSolver_HLLD` `Hydro_RiemannSolver_HLLE` `Hydro_RiemannSolver_Roe` `Hydro_RiemannPredict_Flux` `Hydro_ComputeFlux`
Update 1 function and their function calls: `Hydro_ConFC2PriCC_MHM`
Update 7 functions and their function calls: `Hydro_Con2Pres` `Hydro_Con2Temp` `Hydro_Con2Eint` `Hydro_Con2Entr` `Hydro_Con2Flux` `Hydro_CheckMinEintInEngy` `Hydro_DualEnergyFix`
Update 2 functions and their function calls: `CUFLU_Advance` `CUFLU_FluidSolver_RTVD`
Co-authored-by: He-Feng Hsieh <[email protected]> Thanks for the code review.
Update 6 functions and their function calls: `CPU_SrcSolver` `CUAPI_Asyn_SrcSolver` `CPU_SrcSolver_IterateAllCells` `CUSRC_SrcSolver_IterateAllCells` `Src_Deleptonization` `Src_User_Template` Update an interface function: `SrcFunc_t` Co-authored-by: He-Feng Hsieh <[email protected]> While no source terms exist in the main branch, functions like `Con2Eint()` are used in source terms implemented in other branches.
technic960183
force-pushed
the
neg_passive
branch
from
c907e6b to
fb19e9b
Compare
hyschive
left a comment
hyschive
left a comment
There was a problem hiding this comment.
@technic960183 Looks good. I have only a few minor comments.
src/TestProblem/ELBDM/PlaneWave/Init_TestProb_ELBDM_PlaneWave.cpp
Outdated
Show resolved
Hide resolved
technic960183
left a comment
technic960183
left a comment
There was a problem hiding this comment.
@hyschive Thanks for your reviewing.
Besides the comments you provide, I have also fixed the usage of global variable in Hydro_Con2Dual(). It is caught by Copilot agent mode.
Although Hydro_Con2Dual() is only used by CPU and not by any fluid solver for now, I think it will be good to follow the same convention. So I have also fixed it.
src/TestProblem/ELBDM/PlaneWave/Init_TestProb_ELBDM_PlaneWave.cpp
Outdated
Show resolved
Hide resolved
hyschive
left a comment
hyschive
left a comment
There was a problem hiding this comment.
@technic960183 Final comments added.
Co-authored-by: Hsi-Yu Schive <[email protected]>
Huge thanks to Copilot Agent with Claude Sonnet 4 for catching this!
technic960183
force-pushed
the
neg_passive
branch
from
1452d29 to
acf258c
Compare
technic960183
left a comment
technic960183
left a comment
There was a problem hiding this comment.
@hyschive I have addressed the final comment, renamed the variable and checked the code style.
It passed the regression test I current have.
|
@technic960183 One final comment. Please follow these instructions to update the use of I also noticed that both I'll merge this PR once this final comment is addressed. Thank you for the contribution! |
|
@hyschive I have updated the wiki and compile it on my fork repo. Please take a look. |
5 participants
Summary
This pull request adds support for selectively disabling floor operations on passive scalars.
Rationale
Previously, floor operations were universally applied to all passive scalars. This behavior was limiting and could lead to incorrect results in physical models where some passive scalars are allowed to go negative. By introducing a mechanism to disable floor enforcement for specific passive variables, the code gains more flexibility.
Changes
The changes include adding a new bitwise flag (
PassiveVar_Floor) to specify which passive scalars should be floored, updating relevant functions to use this flag, and extending the data structures and interfaces to support this functionality.PassiveVar_Floorininclude/Global.has a bitwise flag to specify which passive scalars should be subjected to floor operations. Added a new typedefFloorPassive_twith constantsFLOOR_NULL,FLOOR_NO, andFLOOR_YESininclude/Typedef.h.PassiveFloor_Varto theInputPara_tstructure ininclude/HDF5_Typedef.hto store this new flag.const FloorPassive_t FloortoAddField()inInit_Field().Code Updates for Floor Operations
CPU_FluidSolver,Hydro_IsUnphysical,Flu_FixUp_Flux) to accept and utilize theFloorVarparameter for applying floor operations conditionally based on thePassiveVar_Floorflag.Aux_TakeNote()to log the passive scalars that are excluded from floor operations, providing better visibility into the runtime configuration.Tests
Aux_Errorwork when giving invalidFloorPassive_ttoAddField().