-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix/integral symmetry factor #256
base: main
Are you sure you want to change the base?
Conversation
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.
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.
…-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
…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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your output path sneaked in here :-)
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per comment below, I think we could remove all of these and have it always on.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also just for reference from a code convention perspective it should be a bool called uses_symmetry_correction
or something like that)
|
||
public: | ||
/// Default constructor - need to call define afterwards | ||
BoundaryConditions() { is_defined = false; } | ||
|
||
int get_symm() const { return m_symmetry_factor; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abbreviated names are against our conventions :-) Amend to get_symmetry_factor()
Source/GRChomboCore/GRAMRLevel.hpp
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 :) |
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.