Matter: Port Matter classes (with unit tests) #94

julianakwan · 2025-03-01T00:27:18Z

This PR completes my port of the Matter classes and closes #10 . It includes:

Porting MatterCCZ4
Porting the ScalarField and DefaultPotential classes
Porting the calculation of Weyl4_Re and Weyl4_Im for a Matter class
Porting the calculation of the energy density, the momentum density and the spatial stress energy density for Matter classes
Porting the calculation of the relaxation of chi (I didn't know it was going to be depreciated!)
Unit tests for the above
Renaming NewMatterConstraints to just MatterConstraints.
The removal of the Matter directory from the list of ignored files in .lint_ignore

github-actions · 2025-03-01T00:27:32Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/ChiRelaxation.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/MatterCCZ4RHS.impl.hpp
Source/Matter/MatterConstraints.impl.hpp
Source/Matter/MatterWeyl4.impl.hpp
Source/Matter/ScalarField.impl.hpp

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

github-actions · 2025-03-04T15:36:28Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/ChiRelaxation.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/MatterCCZ4RHS.impl.hpp
Source/Matter/MatterConstraints.impl.hpp
Source/Matter/MatterWeyl4.impl.hpp
Source/Matter/ScalarField.impl.hpp

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

mirenradia

Thanks for this PR Juliana.

It looks pretty good but I have some comments. I know there are a fair few of them but I think most of them should be easy to resolve and the more complicated ones I will try and look at myself.

For the GRChombo HDF5 files you have generated for the tests, where is the code you used to create them? I couldn't find a relevant branch in the GRChombo repo.

I don't think the Tests/MatterWeyl4Test/GRChomboEMTensorTest.h5 file is needed/used so could you remove it as well?

Would you also be able to fix the commit history by applying the fixups?

Tests/ChiRelaxationTest/ChiRelaxationTest.cpp

Source/Matter/ChiRelaxation.hpp

Source/Matter/ChiRelaxation.impl.hpp

Tests/MatterWeyl4Test/MatterWeyl4Test.cpp

Tests/TestCases.hpp

Source/Matter/ChiRelaxation.hpp

KAClough · 2025-03-07T12:46:47Z

Source/Matter/EMTensor.hpp

+    /// 3 components for the momentum density: m_11, m_22, m_33
+    /// 6 components for the stress energy tensor (symmetric): s_11, s_12 etc.
+    static inline const amrex::Vector<std::string> extra_var_names = {
+        "m_11, m_22, m_33, s_11, s_12, s_13, s_22, s_23, s_33"};


Yes. We have always used Si and Sij but here that naming might be confusing. We could use J instead, so then it would be J1,J2,J3, S11, S12 etc. Maybe let's leave this for the naming tidy up as it would be best to be consistent throughout the code.

KAClough · 2025-03-07T12:49:00Z

Source/Matter/EMTensor.impl.hpp

This would be good. I always found the passing of intervals in the constructor a bit ugly and confusing.

Source/Matter/MatterConstraints.impl.hpp

KAClough · 2025-03-07T12:56:04Z

Source/Matter/MatterConstraints.hpp

-class [[deprecated("Use new MatterConstraints class in "
-                   "NewMatterConstraints.hpp")]] MatterConstraints
-    : public Constraints
+template <class matter_t> class MatterConstraints : public Constraints


Maybe it is nicer for this to be called ConstraintsWithMatter, and similarly CCZ4RHSWithMatter, Weyl4WithMatter etc but we can do this in the tidy up if it is controversial. The advantage is that the files don't then all start with Matter, which is a bit annoying for tab complete. I also find it more readable.

I would be fine with this change so long as it is done consistently. We might as well do it now given this whole PR is about matter.

Source/Matter/MatterCCZ4RHS.impl.hpp

Source/Matter/MatterWeyl4.impl.hpp

Source/Matter/ScalarField.hpp

github-actions · 2025-03-19T22:20:10Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/CCZ4RHSWithMatter.impl.hpp
Source/Matter/ChiRelaxation.impl.hpp
Source/Matter/ConstraintsWithMatter.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/ScalarField.impl.hpp
Source/Matter/Weyl4WithMatter.impl.hpp

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

github-actions · 2025-04-09T13:51:58Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/CCZ4RHSWithMatter.impl.hpp
Source/Matter/ChiRelaxation.impl.hpp
Source/Matter/ConstraintsWithMatter.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/ScalarField.impl.hpp
Source/Matter/Weyl4WithMatter.impl.hpp

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

github-actions · 2025-04-09T14:01:45Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/CCZ4RHSWithMatter.impl.hpp
Source/Matter/ChiRelaxation.impl.hpp
Source/Matter/ConstraintsWithMatter.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/ScalarField.impl.hpp
Source/Matter/Weyl4WithMatter.impl.hpp

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

github-actions · 2025-04-09T15:13:51Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/CCZ4RHSWithMatter.impl.hpp
Source/Matter/ChiRelaxation.impl.hpp
Source/Matter/ConstraintsWithMatter.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/ScalarField.impl.hpp
Source/Matter/Weyl4WithMatter.impl.hpp

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

github-actions · 2025-04-09T15:33:11Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/CCZ4RHSWithMatter.impl.hpp
Source/Matter/ConstraintsWithMatter.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/ScalarField.impl.hpp
Source/Matter/Weyl4WithMatter.impl.hpp

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

julianakwan · 2025-04-09T16:04:07Z

Hi all - I think I have addressed everyone's comments now (and the lint checks have finally passed). Could you please review this PR again and double check that you are happy with everything?

Main changes are:

ChiRelaxation has been moved to a new branch enhancement/chi_relaxation
Names starting with Matter have been moved to ...WithMatter e.g. MatterCCZ4RHS is now CCZ4RHSWithMatter
Constructor classes involving the Matter object no longer require the Matter object to be passed as an argument because the object is now default constructed from the templated type.
Tests/Common/InitialData.hpp now has a function to initialize test scalar field and conjugate momentum values
I have applied the bugfix from GRChombo to the MovingPunctureGauge. Because it is a matter thing only, it is stored in it's own matter file MovingPunctureGaugeWithMatter.hpp

mirenradia

Thanks for making the changes, Juliana. Just a couple more minor changes.

Source/Matter/EMTensor.hpp

Source/Matter/ConstraintsWithMatter.hpp

Source/Matter/CCZ4RHSWithMatter.hpp

mirenradia · 2025-04-14T12:31:31Z

Source/Matter/EMTensor.hpp

+    static inline const amrex::Vector<std::string> extra_var_names = {
+        "Sij_11, Sij_22, Sij_33, Sij_11, Sij_12, Sij_13, Sij_22, Sij_23, "
+        "Sij_33"};


I think we've agreed on the following names.

Suggested change

static inline const amrex::Vector<std::string> extra_var_names = {

"Sij_11, Sij_22, Sij_33, Sij_11, Sij_12, Sij_13, Sij_22, Sij_23, "

"Sij_33"};

static inline const amrex::Vector<std::string> extra_var_names = {

"j_1, j_2, j_3, S_11, S_12, S_13, S_22, S_23, S_33"};

Based on this comment:

We should make sure our var names are consistent with the emtensor_t struct.

I thought we had decided to make them consistent with the existing names without matter? If you prefer the j's, I can open a pull request to change them both?

Let's just change it to my suggestion in the emtensor_t struct in this PR.

Could you change the emtensor_t struct in CCZ4Geometry.hpp to

template <class data_t> struct emtensor_t { Tensor<2, data_t> S; //!< S_ij = T_ij Tensor<1, data_t> j; //!< j_i = T_ia_n^a data_t trS; //!< trS = S^i_i data_t rho; //!< rho = T_ab n^a n^b };

and modify where this struct appears accordingly?

I opened a new PR for this (#117) because CCZ4Geometry.hpp wasn't part of this original PR. I will change in this PR once that has been accepted and merged.

Source/Matter/CCZ4RHSWithMatter.hpp

Source/Matter/ConstraintsWithMatter.hpp

Source/Matter/ConstraintsWithMatter.impl.hpp

Source/Matter/Weyl4WithMatter.hpp

Source/Matter/Weyl4WithMatter.impl.hpp

github-actions · 2025-04-15T11:53:49Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/CCZ4RHSWithMatter.impl.hpp
Source/Matter/ConstraintsWithMatter.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/ScalarField.impl.hpp
Source/Matter/Weyl4WithMatter.impl.hpp

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

julianakwan · 2025-04-15T12:36:51Z

Hi @mirenradia - I believe I have addressed all your comments now. Can you please take another look at this PR? Thanks!

This will also modify the current implementation of Weyl4::set_up and Weyl4::calculate_mf to be inline (so header can be included with MatterWeyl4 without multiple definitions). It also changes the private members of Weyl4 to be protected, so that MatterWeyl4 can access the derivatives and epsilon3_U_LL and EB fields calculated by Weyl4.

This test will check the calculation of Ham and Mom against a previously calculated set in GRChombo. It will also change the private class members of Constraints to protected and inline the Constraints::set_up and Constraints::calculate_mf so they can be inherited by the matter versions.

Previously, the HDF5 files were written in serial so the CI runs with MPI would fail. Also added MatterWeyl4::set_up and MatterWeyl4::compute_mf functions.

- Calculate MatterWeyl4 and EMTensor by adding these to derive_lst using new MultiFab version of looping over the calculate function - Add set_up function to MatterWeyl4 to set up derive_lst - Add default constructor to ScalarField class

There were some leftover GRChombo characteristics in the calls to Cell in the compute function so I've cleaned these up in this commit. I also fixed a bug where I had mixed up the rhs as Vars and the rhs as an Array4.

This commit adds: - a set_up function to add the energy density, momemtum density and spatial stress-energy tensor as components of derive_lst - compute_mf a wrapper to the compute function which is called by AMReX whenever a derived variable needs to be calculated.

This commit will: - introduce a set_up function that adds "Weyl4_Re" and "Weyl4_Im" to the derive_lst for matter classes. - wrap the compute function inside compute_mf which is called by AMReX whenever the derived variable are needed.

This commit will: - introduce a set_up function for adding "Ham", "Mom1", "Mom2", and "Mom3" (or alternatively "Ham" and "Mom" normalized) to the derive_lst - introduce a compute_mf wrapper that will calculate the above whenever required by AMReX.

This commit will: - Call compute_mf to calculate derived varables - Refactor the HDF5 output to be consistent with the other unit tests (and update the relevent HDF5 files).

This commit will: - Pull out the test for rho from the MatterWeyl4 unit test into its own test case - Test the outputted values of rho calculated by EMTensor against values calculated by GRChombo - Test the outputted values of chi calculated by ChiRelaxation against values calculated by GRChombo - Add the GRChombo .h5 files for comparison - Add these two new test cases to the list in TestCases.hpp

This commit will: - Add an #if statement around HDF5 parts so they are not compiled if USE_HDF5=FALSE - Also, fix Lint errors

This commit will: - fix the bug in GRChombo with the gauge calculation in MatterCCZ4

This commit will: - Add the definition of phi and Pi to the common header file - Refactor the tests to use these definitions. I noticed that the BSSN Matter test and the chi relaxation test were using different initial conditions from the other so I had to update the HDF5 file outputs as well.

This commit will also: - Rename G_newton to G_Newton in the BSSN Matter test

This commit will: - Switch ordering of naming convention, so MatterCCZ4RHS becomes CCZ4RHSWithMatter, MatterConstraints becomes ConstraintsWithMatter, MatterWeyl4 becomes Weyl4WithMatter Matter: Remove matter object from constructor I forgot to remove the matter object in the constructor for CCZ4RHSWithMatter!

There is now a separate branch for the ChiRelaxation class

This will update the naming convention of emtensor_t: - The trace of the spatial stress energy tensor is now trS - The spatial stress energy tensor itself is now S - The momentum vector is now j

github-actions · 2025-04-17T12:44:53Z

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

Source/CCZ4/Weyl4.impl.hpp
Source/Matter/CCZ4RHSWithMatter.impl.hpp
Source/Matter/ConstraintsWithMatter.impl.hpp
Source/Matter/EMTensor.impl.hpp
Source/Matter/ScalarField.impl.hpp
Source/Matter/Weyl4WithMatter.impl.hpp

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

KAClough

I am happy with this PR, I made two minor suggestions for you to consider @julianakwan but I don't feel strongly about them, so whatever you think! Let's get this merged and do the tidy up!

KAClough · 2025-04-25T13:05:23Z

Source/Matter/MovingPunctureGaugeWithMatter.hpp

+/// This class adds the matter terms to the RHS of the gauge equation
+/// for the moving puncture gauge
+
+class MovingPunctureGaugeWithMatter : public MovingPunctureGauge


In the original code, we just added this function into the MovingPunctureGauge class, and didn't inherit to a matter version... I can see it is a bit inconsistent with the structure of the CCZ4/CCZ4Matter classes, but it means fewer files and I don't think it should cause issues, or am I missing something?

I did this because I thought it was tidier to have everything to do with matter in the Source/Matter directory. It doesn't cause any issues otherwise, it was just an issue of consistency so non-matter people can freely ignore the matter stuff if they choose to :)

KAClough · 2025-04-25T13:19:09Z

Source/Matter/ScalarField.impl.hpp

@@ -107,7 +109,8 @@ template <class potential_t>
 template <class data_t, template <typename> class vars_t,
          template <typename> class diff2_vars_t,
          template <typename> class rhs_vars_t>
-void ScalarField<potential_t>::matter_rhs_excl_potential(
+AMREX_GPU_DEVICE AMREX_FORCE_INLINE void
+ScalarField<potential_t>::matter_rhs_excl_potential(


I know that I did this - separating out the rhs and emtensor with and without potential - but now I can't remember why! I think it was to enable doing complex fields, but now we do that without any inheritance from the real fields, which is tidier. This just adds another long function definition that doesn't do much in the body of the function and that makes the code messier. Maybe we should just combine them?

julianakwan added the enhancement New feature or request label Mar 1, 2025

julianakwan requested review from KAClough and mirenradia March 1, 2025 00:27

julianakwan self-assigned this Mar 1, 2025

julianakwan changed the title ~~Enhancement/matter class fix~~ Matter: Port Matter classes (with unit tests) Mar 3, 2025

mirenradia requested changes Mar 6, 2025

View reviewed changes

KAClough reviewed Mar 7, 2025

View reviewed changes

julianakwan force-pushed the enhancement/matter_class_fix branch from 5e527e5 to 2b7a3ce Compare March 19, 2025 22:19

julianakwan force-pushed the enhancement/matter_class_fix branch 2 times, most recently from 1c36aac to a44ef5d Compare April 9, 2025 13:51

julianakwan force-pushed the enhancement/matter_class_fix branch from 50c4e2b to dcf4ae8 Compare April 9, 2025 15:13

julianakwan requested review from mirenradia and KAClough April 9, 2025 15:46

mirenradia requested changes Apr 14, 2025

View reviewed changes

julianakwan force-pushed the enhancement/matter_class_fix branch from da3ee70 to 47a3a61 Compare April 15, 2025 11:53

julianakwan force-pushed the enhancement/matter_class_fix branch from 47a3a61 to 20562ea Compare April 15, 2025 12:14

julianakwan requested a review from mirenradia April 15, 2025 12:36

julianakwan mentioned this pull request Apr 16, 2025

Rename components of emtensor_t for consistency #117

Merged

julianakwan added 2 commits April 17, 2025 11:48

Matter: Port MatterCCZ4 class

70f7c08

Matter: Port Constraints and Weyl4 calculations

f208f0a

julianakwan added 21 commits April 17, 2025 11:48

Matter: Port ScalarField and DefaultPotential classes

e8b276a

Matter: Port ChiRelaxation and EMTensor calculations

576cb7f

Tests: Make MatterWeyl4 and BSSN MatterRHS tests use parallel HDF5

66eec34

Previously, the HDF5 files were written in serial so the CI runs with MPI would fail. Also added MatterWeyl4::set_up and MatterWeyl4::compute_mf functions.

Matter: Fix compute function

950950b

There were some leftover GRChombo characteristics in the calls to Cell in the compute function so I've cleaned these up in this commit. I also fixed a bug where I had mixed up the rhs as Vars and the rhs as an Array4.

Tests: Update unit tests to use compute_mf for derived variables

cc963c1

This commit will: - Call compute_mf to calculate derived varables - Refactor the HDF5 output to be consistent with the other unit tests (and update the relevent HDF5 files).

Matter: Rename NewMatterConstraints to MatterConstraints

aa46dff

Tests: Add pre-processor directives to surround HDF5 includes

e38e239

This commit will: - Add an #if statement around HDF5 parts so they are not compiled if USE_HDF5=FALSE - Also, fix Lint errors

Matter: Apply bugfix 234 from GRChombo

883a5a3

This commit will: - fix the bug in GRChombo with the gauge calculation in MatterCCZ4

Matter: Remove matter object from constructor

02be432

This commit will also: - Rename G_newton to G_Newton in the BSSN Matter test

Matter: Move the gauge calculation re:matter to its own file

9cbdb7d

Matter: Remove ChiRelaxation (to enhancement/chi_relaxation)

1815ca8

There is now a separate branch for the ChiRelaxation class

Matter: Change naming of emtensor components to be consistent

8c30fcb

This will update the naming convention of emtensor_t: - The trace of the spatial stress energy tensor is now trS - The spatial stress energy tensor itself is now S - The momentum vector is now j

julianakwan force-pushed the enhancement/matter_class_fix branch from 20562ea to 8c30fcb Compare April 17, 2025 12:44

KAClough reviewed Apr 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Matter: Port Matter classes (with unit tests) #94

Matter: Port Matter classes (with unit tests) #94

julianakwan commented Mar 1, 2025

github-actions bot commented Mar 1, 2025

github-actions bot commented Mar 4, 2025

mirenradia left a comment

KAClough Mar 7, 2025

KAClough Mar 7, 2025

KAClough Mar 7, 2025

mirenradia Mar 7, 2025

github-actions bot commented Mar 19, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

julianakwan commented Apr 9, 2025 •

edited

Loading

mirenradia left a comment

mirenradia Apr 14, 2025

julianakwan Apr 15, 2025

mirenradia Apr 15, 2025

mirenradia Apr 16, 2025

julianakwan Apr 16, 2025 •

edited

Loading

github-actions bot commented Apr 15, 2025

julianakwan commented Apr 15, 2025

github-actions bot commented Apr 17, 2025

KAClough left a comment

KAClough Apr 25, 2025

julianakwan Apr 25, 2025

KAClough Apr 25, 2025

Matter: Port Matter classes (with unit tests) #94

Are you sure you want to change the base?

Matter: Port Matter classes (with unit tests) #94

Conversation

julianakwan commented Mar 1, 2025

github-actions bot commented Mar 1, 2025

github-actions bot commented Mar 4, 2025

mirenradia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

github-actions bot commented Apr 9, 2025

julianakwan commented Apr 9, 2025 • edited Loading

mirenradia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianakwan Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 15, 2025

julianakwan commented Apr 15, 2025

github-actions bot commented Apr 17, 2025

KAClough left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianakwan commented Apr 9, 2025 •

edited

Loading

julianakwan Apr 16, 2025 •

edited

Loading