Custom refinement ops propagated to fluxes

parthenon-hpc-lab / parthenon

Parthenon AMR infrastructure

https://parthenon-hpc-lab.github.io/parthenon/

Other

105 stars 33 forks source link

Custom refinement ops propagated to fluxes #1100

Closed brryan closed 3 weeks ago

brryan commented 1 month ago

PR Summary

Custom refinement operations enrolled in variable metadata were not being propagated to flux fields if WithFluxes was active. This PR addresses that.

Note that in this model the refinement operations will need to be able to distinguish between the field and the flux field, presumably by branching based on the TopologicalElement.

PR Checklist

[x] Code passes cpplint
[x] New features are documented.
[ ] Adds a test for any bugs fixed. Adds tests for new features.
[x] Code is formatted
[x] 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.
[x] Docs build
[x] (@lanl.gov employees) Update copyright on changed files

brryan commented 4 weeks ago

Nice. Glad this worked. Do we want to propogate the exact same refinement functions? Or do we want to create a second set of refinement functions for fluxes? I guess the topological element is always different, so they're always distinguishable?

Yeah the approach we're taking is to have fluxes get the same refinement functions. As you say they are always distinguishable by topological element, and that is a template parameter, so I don't think there is any cost to doing things anyway, and it is compact. If this seems like a bad design choice though lets discuss that here before merge.

pdmullen commented 3 weeks ago

This PR fixes a breaking change for downstream codes needing custom flux correction routines, therefore, we should likely get it in sooner rather than later...

One comment for future consideration... If we have a field that is NOT Metadata::Independent, NOT Metadata::ForceRemeshComm but it does have Metadata::WithFluxes, it seems strange that we have to enroll all custom PR ops for that field even though the only thing it needs is flux correction.

Actually, is Metadata::WithFluxes enough to trigger IsRefined so that the flux correction is even enrolled?

pdmullen commented 3 weeks ago

Actually, is Metadata::WithFluxes enough to trigger IsRefined so that the flux correction is even enrolled?

the answer is yes: https://github.com/parthenon-hpc-lab/parthenon/blob/ff6a9431a5fd8259aabf0c03a62796774c02eb15/src/interface/metadata.hpp#L451

Yurlungur commented 3 weeks ago

Nice. Glad this worked. Do we want to propogate the exact same refinement functions? Or do we want to create a second set of refinement functions for fluxes? I guess the topological element is always different, so they're always distinguishable?

Yeah the approach we're taking is to have fluxes get the same refinement functions. As you say they are always distinguishable by topological element, and that is a template parameter, so I don't think there is any cost to doing things anyway, and it is compact. If this seems like a bad design choice though lets discuss that here before merge.

Nah it's fine. I think as far as user intent goes, maybe a separate function would be clearer. But as far as implementation "cleanliness" it's unclear what the optimal choice is. There's trade-offs. Go ahead and merge this.