Open mirenradia opened 1 month ago
Some files did not pass the configured checks!
Have any feedback or feature suggestions? Share it here.
Nice - the proposed changes work well! I agree that the stateVariableSetUp should be a part of GRAMRLevel that each PhysicsLevel::variableSetUp can call, since not all applications need the extra CCZ4/BSSN variables.
I guess my question was whether we wanted to make DefaultLevelFactory::variableSetUp()
call this so there would be no need to define a PhysicsLevel::variableSetUp()
if no non-state variables were required?
- It's a bit cumbersome to have to set the parity for all variables even though reflective boundary conditions are not used. It would be nice if there was a separate option that checked for the type of BCs and then if some of them are reflective, look for the parities.
This is a good idea. Let me look at setting some kind of default so that the user doesn't need to set this if they never intend to use reflective BCs.
- Consider changing the definition of variables as enums to be a vector of strings like what AMReX requires when interfacing with plotMultiFab, derive.lst etc. I find myself making a separate vector of strings that just goes over the enums so I can pass variable names to AMReX functions.
I presume you mean StateVariables::names
which is currently of type std::array<std::string, NUM_VARS>
? We went for std::array
at the time for GRChombo as the size is known at compile time but since this isn't really performance critical, I guess there isn't much of a downside to switching to amrex::Vector<std::string>
.
- I think some of the code in PhysicsLevel::derive() can be placed in GRAMRLevel and then called by PhysicsLevel like stateVariableSetUp. For example the Hamiltonian and momentum calculations would be common to any GR application and so will be copied in each GR physics example and this is probably a point of failure (if we update the calculations later on).
We don't want to include the constraint code in GRAMRLevel
since it's GR specific and GRAMRLevel
should be used for non-GR examples too. However, I guess some of the setup code that generates the source multifab can be moved into a GRAMRLevel
function.
I also forgot to mention: some of the 'old' files in the test directory e.g. Tests/CCZ4RHSTest/ADMConformalVars-fdf5a7a.hpp
have been modified but are tagged with a SHA from before the refactoring of the derived variables. Just commenting here so that we remember to update these with the new SHA once this pull request has been merged into the `devel' branch.
I also forgot to mention: some of the 'old' files in the test directory e.g.
Tests/CCZ4RHSTest/ADMConformalVars-fdf5a7a.hpp
have been modified but are tagged with a SHA from before the refactoring of the derived variables. Just commenting here so that we remember to update these with the new SHA once this pull request has been merged into the `devel' branch.
I just had a look at the changes in these files and it's just coming from the renaming UserVariables
-> StateVariables
. I don't think we should rename them and even if we were, I have no idea what we would rename them to.
This PR resolves #41 by moving the variable setup and
derive()
function into the specific level class (well justBinaryBHLevel
for now) rather thanGRAMRLevel
.It also does the following
GRAMRLevel::stateVariableSetUp()
. This needs to be called byPhysicsLevel::variableSetUp()
. We could instead not require this and callstateVariableSetUp()
directly inDefaultLevelFactory::variableSetUp()
?AMREX_USE_HDF5
since we now use HDF5.derive()
to calculate derived quantites on multifabs with ghost cells (which we will need for #2). Previously, for GRChombo we used to fill these ghost cells (using BCs as appropriate) but this would have been very complicated so now we just fill a source multifab with extra ghost cells.vars_parity
andvars_parity_diagnostic
parameters.PhysicsLevel::variableSetUp()
function. MultiFabs are instantiated as required to store them and the component numbers are not fixed.UserVariables
toStateVariables
again for consistency with AMReX.Constraints.[impl.].hpp
and renamesNewConstraints.hpp
toConstraints.hpp
. The former was only kept in GRChombo for backwards compatibility reasons.I still need to make some changes to resolve some of the clang-tidy warnings but, given the size of this PR, I thought I would create it now so that you can start reviewing next week, @julianakwan.