Skip to content
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

Corrections for symmetry factor feature in GRChombo #26

Merged
merged 13 commits into from
Nov 5, 2024

Conversation

the-florist
Copy link
Member

Corrected the params files and any uses of AMRReductions in both of the Examples, to account for the integral symmetry factor feature in GRChombo (see GRChombo Issue No. 197). Specifically:

  1. Added the symmetry_correction flag to the params files (under the Boundary Condition parameters)
  2. Provided the chombo parameter storing the symmetry factor to every instance of the norm and sum AMRReductions functions, in the specificPostTimeStep() function for each example.

Note that the symmetry factor is calculated entirely in the ChomboParameters.h file of the main GRChombo repo.

Corrected the params files and any uses of AMRReductions in
both of the Examples, to account for the integral symmetry factor
feature in GRChombo (see GRChombo Issue No. 197). Specifically:

1. Added the symmetry_correction flag to the params files
(under the Boundary Condition parameters)
2. Provided the chombo parameter storing the symmetry factor
to every instance of the norm and sum AMRReductions functions,
in the specificPostTimeStep() function for each example.

Note that the symmetry factor is calculated entirely in the
ChomboParameters.h file of the main GRChombo repo.
Added the symmetry factor flag to params for both examples,
enabling calls to AMRReductions the ability to correct for
symmetry factor associated with reflective boundary conditions
(see Issue 197).
Copy link
Member

@KAClough KAClough left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Could you also correct the symmetry factors in the python diagnostics scripts (one in each example)? They should not now be needed.

  • One of the intel tests seems to fail - I am not great with these but maybe @SamuelBrady can help...

Examples/BoostedBHComplexScalar/params.txt Outdated Show resolved Hide resolved
@@ -123,6 +123,11 @@ vars_parity = 0 0 0 0 #phi and Pi (Re and Im)
vars_parity_diagnostic = 0 0 0 #chi, rhoLinMom and rhoEnergy
0 0 0 #fluxLinMom, fluxEnergy and sourceLinMom

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would default to true, as I can't imagine people would not want to correct it.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here, default to true and comment out the symmetry_correction =1 line as we do for other defaulted params.

@KAClough
Copy link
Member

KAClough commented Oct 1, 2024

Apologies, the new PR I accepted seems to have broken a few more of the tests, if you could pull this into here (rebase to it?) and fix those at the same time that would be amazing...

@dinatraykova
Copy link
Member

  • Could you also correct the symmetry factors in the python diagnostics scripts (one in each example)? They should not now be needed.
  • One of the intel tests seems to fail - I am not great with these but maybe @SamuelBrady can help...

Changed the intel test as in GRChombo, works now

Copy link
Member

@dinatraykova dinatraykova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good! Tested and works as it should.

@dinatraykova dinatraykova merged commit 41e18f5 into main Nov 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants