Refactor derived variables

[mirenradia]

This PR resolves #41 by moving the variable setup and derive() function into the specific level class (well just BinaryBHLevel for now) rather than GRAMRLevel.

It also does the following

• Adds GRAMRLevel::stateVariableSetUp(). This needs to be called by PhysicsLevel::variableSetUp(). We could instead not require this and call stateVariableSetUp() directly in DefaultLevelFactory::variableSetUp()?
• Adds Weyl4
  • Adds a test which computes this on the random test initial data, outputs to a HDF5 file and then compares it with an HDF5 file produced by the analagous test from GRChombo. The test is skipped when not built with HDF5. I have added HDF5 to the unit tests GH action workflow
• Removes some old unused code paths guarded by AMREX_USE_HDF5 since we now use HDF5.
• Adds the ability for derive() to calculate derived quantites on multifabs with ghost cells (which we will need for Port AMRInterpolator #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.
• Switches to using hardcoded parities rather than letting them be parameters and removes the vars_parity and vars_parity_diagnostic parameters.
• Renames the variable types as follows for consistency with AMReX:
  • evolution -> state
  • diagnostic -> derived
• Removes the notion of fixed diagnostic variables since derived variables are instead defined in the examples PhysicsLevel::variableSetUp() function. MultiFabs are instantiated as
  required to store them and the component numbers are not fixed.
• Renames UserVariables to StateVariables again for consistency with AMReX.
• Adds a test for the Constraints similar to the Weyl4 one.
• Removes the old Constraints.[impl.].hpp and renames NewConstraints.hpp to Constraints.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.

[julianakwan]

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 tested this pull request by merging this branch with the scalar field example that I'm working on. I found a few minor quibbles (so I'll mark it as 'request changes'. Note that I would be happy to leave these as future work on a separate pull request if you would like to merge this branch into develop immediately.)

• 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.
• 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 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).

[mirenradia]

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.

[julianakwan]

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.

[mirenradia]

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.

[github-actions]

This PR modifies the following files which are ignored by .lint-ignore:

Examples/KerrBH/KerrBHLevel.cpp
Examples/KerrBH/KerrBHLevel.hpp
Examples/KerrBH/StateVariables.hpp
Examples/KerrBH/params.txt
Examples/KerrBH/params_cheap.txt
Examples/ScalarField/InitialScalarData.hpp
Examples/ScalarField/ScalarFieldLevel.cpp
Examples/ScalarField/ScalarFieldLevel.hpp
Examples/ScalarField/StateVariables.hpp
Examples/ScalarField/params.txt
Source/AMRInterpolator/AMRInterpolator.hpp
Source/AMRInterpolator/AMRInterpolator.impl.hpp
Source/AMRInterpolator/InterpSource.hpp
Source/AMRInterpolator/InterpolationQuery.hpp
Source/AMRInterpolator/SurfaceExtraction.hpp
Source/AMRInterpolator/SurfaceExtraction.impl.hpp
Source/BoxUtils/SixthOrderDerivatives.hpp
Source/CCZ4/Constraints.impl.hpp
Source/CCZ4/GammaCalculator.hpp
Source/CCZ4/Weyl4.impl.hpp
Source/InitialConditions/BlackHoles/KerrBH.hpp
Source/InitialConditions/ScalarFields/ScalarBubble.hpp
Source/Matter/ChiRelaxation.hpp
Source/Matter/MatterCCZ4RHS.hpp
Source/Matter/NewMatterConstraints.hpp
Source/Matter/ScalarField.hpp
Source/utils/WeylExtraction.hpp
Tests/.AMRInterpolatorTest/AMRInterpolatorTest.cpp
Tests/.AMRInterpolatorTest/AMRInterpolatorTest.inputs
Tests/.AMRInterpolatorTest/Polynomial.hpp
Tests/.AMRInterpolatorTest/StateVariables.hpp
Tests/.AMRInterpolatorTest/UserVariables.hpp
Tests/.BSSNMatterTest/BSSNMatterTest.cpp
Tests/.BSSNMatterTest/StateVariables.hpp
Tests/.MatterWeyl4Test/MatterWeyl4Test.cpp
Tests/.MatterWeyl4Test/StateVariables.hpp
Tests/.SphericalExtractionTest/SetHarmonic.hpp
Tests/.SphericalExtractionTest/SphericalExtractionTestLevel.hpp
Tests/.SphericalExtractionTest/StateVariables.hpp
Tests/.SphericalExtractionTest/UserVariables.hpp
Tests/CCZ4RHSTest/ADMConformalVars-fdf5a7a.hpp
Tests/CCZ4RHSTest/CCZ4RHS-fdf5a7a.hpp
Tests/CCZ4RHSTest/FourthOrderDerivatives-fdf5a7a.hpp
Tests/CCZ4RHSTest/VarsTools-fdf5a7a.hpp

Please consider removing the corresponding patterns from .lint-ignore so that these files can be linted.

[mirenradia]

Following 8bd6e68, this PR now depends on #70 so that should be merged first. This branch is based off the branch in that PR.

[mirenradia]

I should add that 8bd6e68 means that we now use AmrLevel::derive() rather than using our own override. This is possible because AMReX-Codes/amrex#3990 means that we can use the new MultiFab version of DeriveFunc which allows us to use the new, more efficient MultiFab ParallelFor.

[julianakwan]

Thanks so much for changing the variable names to be vectors - I think that was a really handy addition. Also the new set_up and compute_mf functions are nice.

I ran into a few issues running the BinaryBH example though:

1. I can't seem to get the derived variables to print out in the plot files. I'm using inputs.test as the parameter file which contains the lines:

amr.plot_vars = chi # The default is to plot all state variables
amr.derive_plot_vars = Ham Mom # The default is none for derived variables.`

I also added calculate_constraint_norms = true

I think it would be nice if inputs.test (or some other input file) could be modified to contained a working example with the derived variables outputted.

2. A separate issue is if I do amr.derive_plot_vars = ALL then the code exits with a segmentation fault when writing the plot files. It happens in the Weyl4 calculation (I've saved the Backtrace.* files if you want to see?) I think it is because the default value for calculate_constraint_norms is false but ALL makes it calculate everything anyway and there are now too many components in deriv_lst - if I set calculate_constraint_norms = true and amr.derive_plot_vars = ALL then I get every derived variable except Mom.

I think this could be fixed by adding an if statement to SimulationParameters.hpp to check if ALL is set then automatically set calculate_constraint_norms = true as well. I'm not sure about the missing Mom though...

[mirenradia]

I ran into a few issues running the BinaryBH example though:

1. I can't seem to get the derived variables to print out in the plot files. I'm using `inputs.test` as the parameter file which contains the lines:

amr.plot_vars = chi # The default is to plot all state variables
amr.derive_plot_vars = Ham Mom # The default is none for derived variables.`

For this parameter, you need to use the name passed to the derive list. In this case, the name is constraints which contains 4 components. I agree this is a bit confusing. We should document it somewhere.

I also added calculate_constraint_norms = true

This parameter currently doesn't do anything. Previously, in GRChombo it would calculate the L2 norm of the constraint variables and write them to a file (see here). We can now re-implement this but given how long it has taken to merge this PR, I suggest creating an issue (about AMR reductions) and deferring it to a later PR.

I presume you thought this would change what is passed to the a_calc_mom_norm argument in Constraints::set_up()? In GRChombo, it was impossible to change it at runtime as the number of diagnostic variables was hardcoded so it was intended to be hardcoded at the example level. We could make it a parameter but I don't think it's worth it.

I think it would be nice if inputs.test (or some other input file) could be modified to contained a working example with the derived variables outputted.

I already added amr.derive_plot_vars = ALL to params_test.txt but perhaps it would be worth removing these lines from inputs.test to avoid confusion.

2. A separate issue is if I do amr.derive_plot_vars = ALL then the code exits with a segmentation fault when writing the plot files. It happens in the Weyl4 calculation (I've saved the Backtrace.* files if you want to see?) I think it is because the default value for calculate_constraint_norms is false but ALL makes it calculate everything anyway and there are now too many components in deriv_lst - if I set calculate_constraint_norms = true and amr.derive_plot_vars = ALL then I get every derived variable except Mom.

I think this could be fixed by adding an if statement to SimulationParameters.hpp to check if ALL is set then automatically set calculate_constraint_norms = true as well. I'm not sure about the missing Mom though...

I was able to reproduce this. I think the problem comes from saying that Weyl4 only depends on all state variables up to and excluding c_B1 here but then tries to load all of the CCZ4 variables later. Weird that the regression test didn't pick this up. Maybe we should modify it to build with DEBUG = TRUE (or create a matrix like for the unit tests).

[julianakwan]

Thanks! I did a fresh pull from the repo and confirmed that my problems had been fixed.

I presume you thought this would change what is passed to the a_calc_mom_norm argument in Constraints::set_up()?

Yeah, this is exactly what I thought was going on! I agree implementing calculate_constraint_norms should be a separate issue.

Thanks for updating inputs.test also - I think some other parameter files might also have the "old" style of specifying constraint parameters but this should be a separate issue also.