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

Add WithFluxes to IsRefined check #1127

Merged
merged 3 commits into from
Jun 24, 2024
Merged

Add WithFluxes to IsRefined check #1127

merged 3 commits into from
Jun 24, 2024

Conversation

pdmullen
Copy link
Collaborator

@pdmullen pdmullen commented Jun 22, 2024

PR Summary

The fluxes associated with a field marked Metadata::WithFluxes must undergo flux correction with mesh refinement. Parthenon cannot currently enroll custom PR ops on those fluxes if the associated field is marked Metadata::WithFluxes, but not Metadata::Independent, Metadata::FillGhost, or Metadata::ForceRemeshComm.

This PR simply adds WithFluxes to the IsRefined check to circumvent this issue. Technically, this is a bit slimy, because in the example above, the field itself does not undergo any custom-ops, only its associated fluxes, nevertheless, this appears to be the most expedient solution, and IsRefined should not actually be used when packing relevant fields needed for a particular refinement op (please check me on this one, though: https://github.com/search?q=repo%3Aparthenon-hpc-lab%2Fparthenon+isrefined&type=code). If this is too dirty, please feel free to shoot down this PR.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@pdmullen pdmullen mentioned this pull request Jun 22, 2024
12 tasks
@pdmullen pdmullen added the bug Something isn't working label Jun 24, 2024
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

So as a sanity check, I just went through and reminded myself how the bvals stuff works here. For the most part, metadata is used. the ForEachBoundary loop checks for Metadata::FillGhost and Metadata::Flux and does not use IsRefined.

IsRefined is used to help build the map from refinement functions to variables. I think this is needed regardless, so this is all fine. I would like to maybe rename IsRefined though, given we're now changing the meaning... or at least add a comment above the function explaining.

@pdmullen
Copy link
Collaborator Author

So as a sanity check, I just went through and reminded myself how the bvals stuff works here. For the most part, metadata is used. the ForEachBoundary loop checks for Metadata::FillGhost and Metadata::Flux and does not use IsRefined.

IsRefined is used to help build the map from refinement functions to variables. I think this is needed regardless, so this is all fine. I would like to maybe rename IsRefined though, given we're now changing the meaning... or at least add a comment above the function explaining.

Thanks for the sanity check. I had also come to the same conclusion. I agree with your comment regarding renaming IsRefined. d7c771cd7c771c renames IsRefined -> HasRefinementOps.

@pdmullen pdmullen enabled auto-merge June 24, 2024 19:53
@pdmullen pdmullen merged commit 606cb56 into develop Jun 24, 2024
50 checks passed
@pdmullen pdmullen deleted the pdmullen/meta-isref-fix branch June 25, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants