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

Refactor Region Size #918

Merged
merged 11 commits into from
Aug 17, 2023
Merged

Refactor Region Size #918

merged 11 commits into from
Aug 17, 2023

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Aug 10, 2023

PR Summary

This PR does a small refactor of RegionSize and related code, nothing exciting. This shouldn't change anything about how the code behaves, but removes a little repetition. I am mainly doing this in anticipation of allowing for varying block size in geometric multigrid levels, so that the mesh can be coarsened to the greatest degree possible.

PR Checklist

  • Code passes cpplint
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

Copy link
Collaborator

@bprather bprather left a comment

Choose a reason for hiding this comment

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

LGTM. Someday I'll switch to .symmetry in KHARMA, seems clean

@lroberts36 lroberts36 enabled auto-merge (squash) August 17, 2023 19:26
@lroberts36 lroberts36 merged commit 6f3c5dc into develop Aug 17, 2023
49 checks passed
@pgrete
Copy link
Collaborator

pgrete commented Aug 17, 2023

Sorry for being late to the show.
Two post-merge question:
a) This should get a "breaks downstream" label, doesn't it? We're directly accessing pmesh->mesh_size.x1max - pmesh->mesh_size.x1min; in AthenaPK.
b) What the intention behind the symmetry naming? Maybe I'm too tired, but it's not immediately obvious to me. As far as I was able to follow the current intention it's sth like IsND<DIR>.

@lroberts36
Copy link
Collaborator Author

@pgrete: a) Yes, I guess that it should have "breaks downstream". I can put in a small PR to move the addition to the Changelog.

b) In many places in the code, a check for nx2 > 1 was used to determine that a direction was not a symmetry direction. For GMG, I would like to have blocks that are size one but still have ghost cells in those directions. symmetry just replaces those checks and currently symmetry just get set for each direction by performing the check !(nx > 1)

@pgrete
Copy link
Collaborator

pgrete commented Aug 18, 2023

@pgrete: a) Yes, I guess that it should have "breaks downstream". I can put in a small PR to move the addition to the Changelog.

I think we can drive-by add this in another PR. No rush to create an extra PR (from my point of view)

b) In many places in the code, a check for nx2 > 1 was used to determine that a direction was not a symmetry direction. For GMG, I would like to have blocks that are size one but still have ghost cells in those directions. symmetry just replaces those checks and currently symmetry just get set for each direction by performing the check !(nx > 1)

Maybe it's just a wording thing with respect to different fields/backgrounds.
For example, when checking for "do we we need boundary condition in the x3 dim" then the original check nx3 >1 in my mind check for "does this simulation has cells in the third dimension".
My main confusion is probably around that (in my mind) different types of symmetry properties exists that may or may not be orthogonal to the question whether a simulation is 1D, 2D or 3D.

@lroberts36
Copy link
Collaborator Author

@pgrete:

Maybe it's just a wording thing with respect to different fields/backgrounds. For example, when checking for "do we we need boundary condition in the x3 dim" then the original check nx3 >1 in my mind check for "does this simulation has cells in the third dimension". My main confusion is probably around that (in my mind) different types of symmetry properties exists that may or may not be orthogonal to the question whether a simulation is 1D, 2D or 3D.

Conceptually, we need a flag that says whether or not we want to have ghost cells and apply boundary conditions in a given direction. Using nx1>1 as a proxy for this doesn't really work when the block size varies on GMG levels and may get down to a one^3 block but still require ghosts and BCs for consistency with other blocks in the hierarchy.

Beyond that, I think it is a wording thing and I am happy to change the nomenclature if there is something clearer to others. My thinking for that name was that a simulation either possesses a translation symmetry in a given direction (so all derivatives in that direction are zero and it does not require ghost zones) or it doesn't possess a translation symmetry in that direction and requires ghost zones. I agree that symmetry may be too broad, since a direction can possess other symmetries than translation (e.g. reflection), so maybe we could use translation_symmetry although that is a bit verbose. There is a distinction between this and a truly 2D or 1D simulation (which would formally require defining only quantities on one set of cell faces, edges, and nodes, or only on edges and nodes, respectively), although in practice it usually shouldn't matter I guess.

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.

5 participants