Bugfix/integral symmetry factor #256

the-florist · 2024-09-01T17:11:16Z

This PR Closes #197 by adding a symmetry factor variable to ChomboParameters, which calculates the symmetry factor 2^n, where n is the number of dimensions which have been halved in memory, whenever reflective boundary conditions are used. This symmetry factor is then used to normalise integrated quantities calculated by the AMRReductions sum() and norm() functions. Note that the flag has been added to the params files for all examples (in the Boundary Conditions section), even if reflective boundaries are not used. The flag controls whether the symmetry factor is used in the case where reflective boundaries are chosen, and is defaulted to be off.

Additionally, this feature MAY BREAK YOUR CODE. Any instances of the AMRReductions' sum() and norm() which do not appear in the main branch must have the m_p.symmetry_factor variable added to the argument list, IMMEDIATELY AFTER the enum holding the field name, i.e. in the second argument place. The symmetry factor argument is NOT defaulted, so if it is missing from one of these function calls, a compiler error should be thrown. Note that the symmetry factor argument must be included even if no reflective boundaries are used, and even if the flag controlling whether the integrated quantity is normalised is turned off.

Note that I have also changed some of the previous AH finder parameters in params_very_cheap.txt to reflect the choices made for params.txt, for the BinaryBH example, as the code threw errors with the previous set of parameters.

1. The symmetry correction flag is read in ChomboParameters. It is defaulted to "off" if the parameter is not provided. 2. If symmetry correction is flag is on, the symmetry factor is calculated based off of the number of hi or lo boundaries chosen to be reflective (choice 2). 3. The symmetry factor is a chombo parameter, and must be given to any AMRReduction function which integrates over the entire box (currently norm and sum). The symmetry factor is defaulted to 1 if it is not provided to the AMRReduction scheme. 4. The result of sum or norm is multiplied by the symmetry factor before it is returned, in the case of a single variable. The symmetry factor is provided correctly to AMRReductions for the BinaryBH example ONLY in this commit.

Fixed a few bugs with how I had first implemented the symmetry factor: 1. Moved the symmetry factor argument in norm and sum ahead of the defaulted variables. 2. Changed type of the symmetry factor variable in sum and norm to int, and cast it to double before applying to the final result. 3. Changed error in the name of the variable in norm.

KAClough · 2024-09-02T16:55:19Z

I have asked @dinatraykova to review so lets see what she thinks, but I would favour not breaking old code and making it know about the symmetry factor when you construct the AMR reductions class via m_gr_amr - this ought to be able to know about the symmetries of the grid, and then the use of symmetry could be defaulted to true.

…on a new branch...

Created a series of subroutines to directly read in the symmetry factor associated with using reflective boundary conditions in the GRAMR class, which can then be drawn out for use by the sum() and norm() AMRReductions functions. 1. The symmetry factor is calculated in BoundaryConditions as a parameter, then assigned to a private BoundaryConditions variable. 2. A GRAMRLevel subroutine, get_symm(), calls the BoundaryConditions subroutine to return the private symmetry factor variable. 3. AMRReductions reads in the symmetry factor from get_gramrlevels in the same style as m_coarsest_dx. 4. The symmetry factor is applied to the norm() and sum() results immediately before they are returned.

…-symmetry-factor This commit is to fix the conflicts between the two possible solutions -- either calculating the symmetry factor as a Chombo parameter, or calculaing it entirely in BoundaryConditions and passing it through GRAMR.

…GRTLCollaboration/GRChombo into bugfix/integral-symmetry-factor

Created a series of subroutines to directly read in the symmetry factor associated with using reflective boundary conditions in the GRAMR class, which can then be drawn out for use by the sum() and norm() AMRReductions functions. 1. If the symmetry_correction flag in params is turn on, the symmetry factor is calculated in BoundaryConditions as a parameter, then assigned to a private BoundaryConditions variable. 2. A GRAMRLevel subroutine, get_symm(), calls the BoundaryConditions subroutine to return the private symmetry factor variable. 3. AMRReductions reads in the symmetry factor from get_gramrlevels in the same style as m_coarsest_dx. 4. The symmetry factor is applied to the norm() and sum() results immediately before they are returned.

…class.

the-florist · 2024-09-10T14:26:33Z

Note that this version does NOT break pre-existing code by requiring users to input a new Chombo parameter into instances of sum() and norm(), but rather collects the symmetry factor directly from BoundaryConditions and automatically applies it if the flag controlling this feature is turned on. If the flag is not included in the params file, the symmetry factor is automatically set to 1, and no error is thrown.

KAClough

This is great! A few minor amendments but I like how you added it via the boundary params. Once you are done I will check it runs ok and then we can merge.

KAClough · 2024-10-01T20:53:59Z

Examples/BinaryBH/params_very_cheap.txt

@@ -9,7 +9,7 @@
 verbosity = 0

 # location / naming of output files
-# output_path = "" # Main path for all files. Must exist!
+output_path = "/nfs/st01/hpc-gr-epss/eaf49/PR-dump" # Main path for all files. Must exist!


Your output path sneaked in here :-)

KAClough · 2024-10-01T20:54:18Z

Examples/BinaryBH/params_very_cheap.txt

@@ -108,6 +108,11 @@ vars_parity            = 0 0 4 6 0 5 0    #chi and hij
 vars_parity_diagnostic = 0 1 2 3          #Ham and Mom
                         0 7              #Weyl

+# Additionally, if reflective boundaries selected, 
+# choose if symmetry factor will be applied to integrals.
+# 0 = false (default), 1 = true


As per comment below, I think we could remove all of these and have it always on.

KAClough · 2024-10-01T20:58:47Z

Source/BoxUtils/AMRReductions.impl.hpp

@@ -109,7 +110,7 @@ Real AMRReductions<var_t>::norm(const Interval &a_vars,
            pow(m_domain_volume, 1.0 / static_cast<double>(a_norm_exponent));
    }

-    return norm;
+    return norm * static_cast<double>(m_symmetry_factor);


I think this needs to take account of the value of a_normalize_by_volume - if that is true then we don't need to scale the norm, as it is a per volume measure. In the case where that is false then this is correct.

KAClough · 2024-10-01T21:03:26Z

Source/GRChomboCore/BoundaryConditions.cpp

@@ -141,6 +142,23 @@ void BoundaryConditions::params_t::read_params(GRParmParse &pp)

    if (reflective_boundaries_exist)
    {
+        int symmetry_correction = 0;
+        pp.load("symmetry_correction", symmetry_correction, 0);


I would actually be tempted just to remove the symmetry_correction variable, and always work out this symmetry factor. I can't imagine going forward that someone will not want to use it. This also means we don't need to change all the params files for another variable that people will just ignore or find confusing.

(Also just for reference from a code convention perspective it should be a bool called uses_symmetry_correction or something like that)

KAClough · 2024-10-01T21:06:06Z

Source/GRChomboCore/BoundaryConditions.hpp


  public:
    /// Default constructor - need to call define afterwards
    BoundaryConditions() { is_defined = false; }

+    int get_symm() const { return m_symmetry_factor; }


Abbreviated names are against our conventions :-) Amend to get_symmetry_factor()

KAClough · 2024-10-01T21:06:25Z

Source/GRChomboCore/GRAMRLevel.hpp

@@ -177,6 +177,10 @@ class GRAMRLevel : public AMRLevel, public InterpSource<>

    // direction irrelevant, but relevant for InterpSource
    ALWAYS_INLINE double get_dx(int dir = 0) const { return m_dx; };
+    ALWAYS_INLINE int get_symm(int dir = 0) const


Ditto - no abbreviation here.

1. Removed symmetry_correction flag, making the symmetry factor calculation automatic. 2. Moved application of symmetry factor in AMRReductions::norm function into the logic block controlled by the a_normalize_by_volume flag, so that only when normalising by the volume will the symmetry factor be applied. 3. Changed get_symm() function names to get_symmetry_factor() (coding convention). 4. Removed file path in params_very_cheap.txt in BinarBH example.

the-florist · 2024-10-14T16:59:09Z

Thank you for the comments! The latest commit should have fixed all of these issues. Feel free to test on your system when you have a moment and let me know if something unexpectedly breaks :)
-Ericka

the-florist added 4 commits August 31, 2024 14:53

Added symmetry correction flag to all params in all Examples

cbd4298

Adding params changes, so I can bugfix separately

4c790f6

KAClough requested a review from dinatraykova September 2, 2024 16:49

KAClough added the bug Something isn't working label Sep 2, 2024

the-florist added 16 commits September 3, 2024 16:38

Trying to add symmetry factor from GRAMR but I think I need to start …

4aef8c9

…on a new branch...

Merge branch 'bugfix/integral-symmetry-factor' of https://github.com/…

f9fa5f3

…GRTLCollaboration/GRChombo into bugfix/integral-symmetry-factor

Removed symmetry argument from uses of AMRReductions

cd53124

Fixed clang tidy warnings

ae197d4

Fixed clang tidy warnings

1eb4cd3

Merge branch 'bugfix/integral-symmetry-factor' of https://github.com/…

dd57179

…GRTLCollaboration/GRChombo into bugfix/integral-symmetry-factor

Clang formatting changes, after running clang-format locally

d6808dc

Fixed typo on line 40 of BoundaryConditions.cpp

d01cdf7

Added default to the symmetry_correction flag

8bbb859

Pre-pull commit

a4e72e0

AH-solver-bug

327021d

Removal of old Chombo Parameter function argument from AMRReductions …

0bee0fa

…class.

KAClough reviewed Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/integral symmetry factor #256

Bugfix/integral symmetry factor #256

the-florist commented Sep 1, 2024

KAClough commented Sep 2, 2024 •

edited

Loading

the-florist commented Sep 10, 2024

KAClough left a comment

KAClough Oct 1, 2024

KAClough Oct 1, 2024

KAClough Oct 1, 2024

KAClough Oct 1, 2024

KAClough Oct 1, 2024

KAClough Oct 1, 2024

KAClough Oct 1, 2024

the-florist commented Oct 14, 2024

Bugfix/integral symmetry factor #256

Are you sure you want to change the base?

Bugfix/integral symmetry factor #256

Conversation

the-florist commented Sep 1, 2024

KAClough commented Sep 2, 2024 • edited Loading

the-florist commented Sep 10, 2024

KAClough left a comment

Choose a reason for hiding this comment

KAClough Oct 1, 2024

Choose a reason for hiding this comment

KAClough Oct 1, 2024

Choose a reason for hiding this comment

KAClough Oct 1, 2024

Choose a reason for hiding this comment

KAClough Oct 1, 2024

Choose a reason for hiding this comment

KAClough Oct 1, 2024

Choose a reason for hiding this comment

KAClough Oct 1, 2024

Choose a reason for hiding this comment

KAClough Oct 1, 2024

Choose a reason for hiding this comment

the-florist commented Oct 14, 2024

KAClough commented Sep 2, 2024 •

edited

Loading