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

Provide more flexible ways to add subset MeshBlockData objects to DataCollections #921

Merged
merged 36 commits into from
Aug 16, 2023

Conversation

jdolence
Copy link
Collaborator

@jdolence jdolence commented Aug 11, 2023

PR Summary

There are lots of examples where it would be useful to have more flexibility in adding new MeshBlockData objects to DataCollections that are slices of other MeshBlockData objects (like "base"). We already sort of support this via the overload to DataCollection::Add that includes a list of names of variables, but in its current form this is of limited use in practice. This PR attempts to make this much more flexible and useful.

The main features included in this PR are:

  • New overloads to add MeshBlockData or MeshData objects to DataCollections. The preexisting versions that take (label, source) or (label, source, var_names) still exist and function as before. In addition, an AddShallow function (with the same overloads) was added that yields a shallow copy, even for non-Metadata::OneCopy fields.
  • It now works to Add directly to the Mesh-level DataCollection. When Add is called with a MeshData source object, the new MeshBlockData objects that get created get added to the MeshBlock level DataCollections. The implementation is a bit of a hack, but it works for now and makes things much cleaner downstream.
  • StateDescriptor now has an overloaded GetVariablesNames member function that returns a std::vector<std::string> of variable names given a list of names, Metadata::FlagCollectoin, and/or a list of sparse ids. This is meant to produce the names that get passed into the DataCollection::Add* calls, so provides the means of creating new Mesh*Data objects that are filtered on, for example, Metadata and/or sparse ids.
  • GetVariableNames overloads were also added to Mesh, which just forwards the call on to the resolved_state StateDescriptor functions. This again is for convenience so that downstream codes don't have to pull out the resolved_state when they just want a list of names from the simulation state as a whole.
  • Parthenon will now automatically add a Metadata flag with the same name as the StateDescriptor the field is added to every field, sparse pool, and swarm. StateDescriptor stashes the new flag in a Param and provides a function to retrieve the flag for convenient access later.
  • Given a list of names, a Metadata::FlagCollection, and/or sparse ids, there are new functions that additionally take two MeshBlockData pointers or two MeshData pointers and return the unique ids of the variables that satisfy the filters and exist in both objects. This is required when trying to work with Mesh*Data objects that may have different fields present. With just the intersection of unique ids, you can write functions that can, for example, add values from one MeshData object into the corresponding fields in another MeshData object quite straightforwardly.
  • New PackDescriptor code that allows you to produce a SparsePack given a list of unique ids.

With all this stuff in place, you can do things like make registers for the RHSs of subsets of equations. We have integrated this branch into our downstream code and successfully make use of all these features.

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
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@jdolence
Copy link
Collaborator Author

@pdmullen @bprather @Yurlungur @lroberts36 @pgrete I think this one is ready to review. I'm going to leave Draft in place until I do a bit more testing downstream, but so far things look good so I suspect I'll be removing Draft very soon.

Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

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

LGTM, comments aren't blocking.

src/amr_criteria/amr_criteria.hpp Outdated Show resolved Hide resolved
src/interface/data_collection.cpp Show resolved Hide resolved
src/interface/meshblock_data.cpp Show resolved Hide resolved
src/interface/meshblock_data.hpp Outdated Show resolved Hide resolved
src/interface/metadata.hpp Outdated Show resolved Hide resolved
src/interface/state_descriptor.cpp Outdated Show resolved Hide resolved
src/interface/state_descriptor.cpp Outdated Show resolved Hide resolved
src/interface/state_descriptor.cpp Outdated Show resolved Hide resolved
src/interface/state_descriptor.cpp Outdated Show resolved Hide resolved
jdolence and others added 3 commits August 15, 2023 13:21
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.

LGTM. This will be a nice improvement. Do you need changelog + formatting? Also remove the draft prefix?

src/interface/data_collection.cpp Outdated Show resolved Hide resolved
src/interface/data_collection.cpp Outdated Show resolved Hide resolved
src/interface/data_collection.cpp Outdated Show resolved Hide resolved
src/interface/meshblock_data.cpp Show resolved Hide resolved
Comment on lines -138 to -163
// Constructor for getting sub-containers
// the variables returned are all shallow copies of the src container.
// Optionally extract only some of the sparse ids of src variable.
template <typename T>
MeshBlockData<T>::MeshBlockData(const MeshBlockData<T> &src,
const std::vector<std::string> &names,
const std::vector<int> &sparse_ids) {
CopyFrom(src, true, names, {}, sparse_ids);
}

// TODO(JMM): Add constructor that takes unique IDs
template <typename T>
MeshBlockData<T>::MeshBlockData(const MeshBlockData<T> &src,
const std::vector<MetadataFlag> &flags,
const std::vector<int> &sparse_ids) {
CopyFrom(src, true, {}, flags, sparse_ids);
}

// provides a container that has a single sparse slice
template <typename T>
std::shared_ptr<MeshBlockData<T>>
MeshBlockData<T>::SparseSlice(const std::vector<int> &sparse_ids) const {
auto c = std::make_shared<MeshBlockData<T>>();
c->CopyFrom(*this, true, {}, {}, sparse_ids);
return c;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

No constructor overloads, always use copy now? That's cleaner from a self-describing interface point of view I think

src/interface/meshblock_data.hpp Show resolved Hide resolved
src/utils/unique_id.cpp Show resolved Hide resolved
Copy link
Collaborator

@pdmullen pdmullen left a comment

Choose a reason for hiding this comment

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

LGTM!

src/interface/state_descriptor.hpp Outdated Show resolved Hide resolved
@jdolence jdolence changed the title Draft: Provide more flexible ways to add subset MeshBlockData objects to DataCollections Provide more flexible ways to add subset MeshBlockData objects to DataCollections Aug 16, 2023
@jdolence
Copy link
Collaborator Author

@pgrete I think this one is ready to go. Did you want to look through it first or should I just pull the trigger. I'm pretty sure this won't break anything for you downstream.

@bprather
Copy link
Collaborator

After pulling and fiddling a bunch I like all this as is too.

It's prompted me to dump some more of the other functionality I was duplicating in KHARMA into Parthenon too, I'll open a separate PR with the additions.

@jdolence
Copy link
Collaborator Author

@pgrete didn't give you much time to respond, but I'm going to pull the trigger. We have a least two separate codes that have confirmed backwards compatibility, so I'm pretty confident this won't break anything for you. Obviously, if you have comments on anything in this after it's merged, we can work on getting them addressed.

@jdolence jdolence merged commit eecd058 into develop Aug 16, 2023
45 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.

5 participants