Allow for accessing flux `Metadata` from `Metadata::WithFluxes` `Metadata` #1161

lroberts36 · 2024-08-26T19:50:35Z

PR Summary

This PR adds a std::shared_ptr<Metadata> flux_metadata member to Metadata that is initialized in the Metadata constructor when the flag Metadata::WithFluxes is set. A new Metadata constructor is added that takes a list of flags for both the original field and the flux field, as well as separate prolongation and restriction operations for the base field and the flux field (but all of the old constructors behavior is unchanged). The easier way to set non-default properties of the flux Metadata is to get access to it through the method Metadata::GetSPtrFluxMetadata() and Set and Unset flags there and register refinement operations, e.g.

Metadata m({Metadata::Face, Metadata::WithFluxes, Metadata::Vector}, {1, 2});
auto pm_flux = m.GetSPtrFluxMetadata(); 
pm_flux.Unset(Metadata::OneCopy); 
pm_flux.RegisterRefinementOps<OpPro, OpRes, OpInternalPro>();

This PR also adds a flag Metadata::CellMemAligned, which causes a field to have memory allocated with the same size as a cell variable no matter what its topological type is. This is set by default for fluxes associated with cell centered fields (to ensure backwards compatibility). Obviously, having this flag set means that face, edge, and nodal fields will be missing one layer of ghosts on some sides, but having it set can allow for vectorization in some downstream codes.

Finally, (sorry) this PR includes a couple of small changes to sparse pack cache checking and to boundary communication. These are just performance enhancements that got merged into this branch by mistake and it was easier to leave them in than jump through a bunch of git hoops. If people are strongly opposed to including them here, let me know.

PR Checklist

Yurlungur

LGTM! Should we have a test?

lroberts36 · 2024-08-26T19:53:38Z

LGTM! Should we have a test?

Let's have a Sam based test first. Once that passes I can add one to the unit tests.

…formance-updates

lroberts36 · 2024-08-29T17:22:03Z

@Yurlungur: Maybe this isn't the "right" way to do things. It seems like we will have to add a second flag for many options just so that it can pass through to the fluxes. Maybe we should really be making a Metadata constructor that takes two vectors of metadata flags and uses the second one to internally construct a second Metadata object for the flux field. What do you think?

Also, I added a flag Metadat::CellMemAligned in this PR that allows you to explicitly choose what the allocated size of an array is (so that you can do the Riot vectorization tricks more easily in the FLD solver).

…om/parthenon-hpc-lab/parthenon into lroberts36/performance-updates

Yurlungur · 2024-08-29T19:40:26Z

@Yurlungur: Maybe this isn't the "right" way to do things. It seems like we will have to add a second flag for many options just so that it can pass through to the fluxes. Maybe we should really be making a Metadata constructor that takes two vectors of metadata flags and uses the second one to internally construct a second Metadata object for the flux field. What do you think?

I think this is a nice idea... though I would suggest it be optional.

Also, I added a flag Metadat::CellMemAligned in this PR that allows you to explicitly choose what the allocated size of an array is (so that you can do the Riot vectorization tricks more easily in the FLD solver).

👍

lroberts36 · 2024-08-29T19:59:33Z

@Yurlungur: Maybe this isn't the "right" way to do things. It seems like we will have to add a second flag for many options just so that it can pass through to the fluxes. Maybe we should really be making a Metadata constructor that takes two vectors of metadata flags and uses the second one to internally construct a second Metadata object for the flux field. What do you think?

I think this is a nice idea... though I would suggest it be optional.

It would have to be optional for backward compatibility, but the idea would be to just have a set of default Metadata (which we effectively have now) and allow that to be overridden with by the new constructor.

lroberts36 · 2024-09-04T21:16:59Z

src/bvals/comms/boundary_communication.cpp

@@ -213,7 +213,7 @@ TaskStatus ReceiveBoundBufs(std::shared_ptr<MeshData<Real>> &md) {
      [&all_received](auto pbuf) { all_received = pbuf->TryReceive() && all_received; });

  int ibound = 0;
-  if (Globals::sparse_config.enabled) {
+  if (Globals::sparse_config.enabled && all_received) {


This changes to allocating fields only after all boundary buffers are received. This should have no impact on code behavior, but can have a performance impact in some cases. The change is unrelated to the rest of the PR.

lroberts36 · 2024-09-04T21:18:22Z

src/interface/sparse_pack_base.hpp

@@ -138,7 +138,7 @@ struct PackDescriptor {
  // default constructor needed for certain use cases
  PackDescriptor()
      : nvar_groups(0), var_group_names({}), var_groups({}), with_fluxes(false),
-        coarse(false), flat(false), identifier("") {}
+        coarse(false), flat(false), identifier(""), nvar_tot(0) {}


This just counts the total number of variables included in the pack at the time of PackDescriptor construction so it doesn't have to be redone every time the cache is checked to see if a given pack is stale. Unrelated to the bulk of this PR.

lroberts36 · 2024-09-04T21:19:29Z

src/interface/sparse_pack_base.cpp

+        astat[idx++] =
+            uid_map.count(uid) > 0 ? (uid_map.at(uid))->GetAllocationStatus() : -1;


Try to limit branching and preallocate the std::vector for SparsePack cache checking. Unrelated to the rest of the PR.

Yurlungur

This all looks good to me.

lroberts36 added 2 commits August 26, 2024 13:48

done

32a0d24

changelog

fb80247

Yurlungur approved these changes Aug 26, 2024

View reviewed changes

lroberts36 added 5 commits August 28, 2024 19:23

Add CellMemAligned flag

5c9f88c

Only allocate after all received

c664973

Maybe speed up allocation status check

7ee6475

Actually include the flag

f31cce4

Merge branch 'lroberts36/add-non-one-copy-fluxes' into lroberts36/per…

3eac37d

…formance-updates

lroberts36 added 2 commits August 29, 2024 13:30

Merge branch 'develop' into lroberts36/add-non-one-copy-fluxes

19f7d85

Merge branch 'lroberts36/add-non-one-copy-fluxes' of https://github.c…

9728938

…om/parthenon-hpc-lab/parthenon into lroberts36/performance-updates

Yurlungur approved these changes Aug 29, 2024

View reviewed changes

lroberts36 added 4 commits September 4, 2024 11:29

Merge branch 'develop' into lroberts36/add-non-one-copy-fluxes

dc4efa9

format and lint

8e8f201

Make flux metadata accessible at WithFlux variable creation point

3ff2e8a

Add some documentation

2977980

lroberts36 changed the title ~~Small: Allow for fluxes that are not Metadata::OneCopy~~ Small: Allow for accessing flux Metadata from Metadata::WithFluxes Metadata Sep 4, 2024

lroberts36 commented Sep 4, 2024

View reviewed changes

update changelog

917bf35

lroberts36 requested review from Yurlungur, brryan, pgrete and pdmullen September 4, 2024 21:20

lroberts36 changed the title ~~Small: Allow for accessing flux Metadata from Metadata::WithFluxes Metadata~~ Allow for accessing flux Metadata from Metadata::WithFluxes Metadata Sep 4, 2024

ensure backward compatibility

d5c653d

pdmullen approved these changes Sep 4, 2024

View reviewed changes

Yurlungur approved these changes Sep 12, 2024

View reviewed changes

Merge branch 'develop' into lroberts36/add-non-one-copy-fluxes

fc82310

lroberts36 enabled auto-merge September 12, 2024 16:42

lroberts36 merged commit 864f5c8 into develop Sep 12, 2024
53 checks passed

Yurlungur deleted the lroberts36/add-non-one-copy-fluxes branch June 20, 2025 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow for accessing flux `Metadata` from `Metadata::WithFluxes` `Metadata` #1161

Allow for accessing flux `Metadata` from `Metadata::WithFluxes` `Metadata` #1161

Uh oh!

lroberts36 commented Aug 26, 2024 •

edited

Loading

Uh oh!

Yurlungur left a comment

Uh oh!

lroberts36 commented Aug 26, 2024

Uh oh!

lroberts36 commented Aug 29, 2024

Uh oh!

Yurlungur commented Aug 29, 2024

Uh oh!

lroberts36 commented Aug 29, 2024

Uh oh!

lroberts36 Sep 4, 2024

Uh oh!

lroberts36 Sep 4, 2024

Uh oh!

lroberts36 Sep 4, 2024

Uh oh!

Yurlungur left a comment

Uh oh!

Uh oh!

Uh oh!

		astat[idx++] =
		uid_map.count(uid) > 0 ? (uid_map.at(uid))->GetAllocationStatus() : -1;

Allow for accessing flux Metadata from Metadata::WithFluxes Metadata #1161

Allow for accessing flux Metadata from Metadata::WithFluxes Metadata #1161

Uh oh!

Conversation

lroberts36 commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Yurlungur left a comment

Choose a reason for hiding this comment

Uh oh!

lroberts36 commented Aug 26, 2024

Uh oh!

lroberts36 commented Aug 29, 2024

Uh oh!

Yurlungur commented Aug 29, 2024

Uh oh!

lroberts36 commented Aug 29, 2024

Uh oh!

lroberts36 Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

lroberts36 Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

lroberts36 Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

Yurlungur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Allow for accessing flux `Metadata` from `Metadata::WithFluxes` `Metadata` #1161

Allow for accessing flux `Metadata` from `Metadata::WithFluxes` `Metadata` #1161

lroberts36 commented Aug 26, 2024 •

edited

Loading