Remove ambiguity in package names.

[Yurlungur]

PR Summary

This is related to issue #423 and fixes (in my opinion) an underlying design flaw. In a downstream code, @clellsolomon discovered that there are really two names for each package. There's the label set in the StateDescriptor constructor, e.g.,

auto pkg = std::make_shared<StateDescriptor>(name1);

and then there's the key in the std::unordered_map used for Packages_t:

Packages_t packages;
packages[name2] = pkg;

It's implicitly assumed that name1 and name2 are the same, but it's not enforced anywhere. And even if we enforced it, it's strange that we require the user to enter this name twice. Here I propose and implement one solution, which I think is the preferred one. I have changed Packates_t from a std::unordered_map to it's own class, which adds a package with the same key as the label in the package. E.g., you now call:

packages.Add(pkg); // registers the package with name pkg->label()

and you can access the package via

packages.Get("package name");

which has bounds checking, so an error is thrown if the package doesn't exist.

I like this solution because it:

1. Removes the aforementioned ambiguity with regards to package names
2. "Encapsulates" package names to some degree to the package itself, rather than the application. I.e., you can imagine having an application that initializes 3 or 4 packages. The packages can be swapped in or out and the application doesn't necessarily need to know what they're called.

The disadvantages of this solution are:

1. It is API breaking because I removed the operator[] from Packages_t and replaced it with Add and Get. However, the changes to downstream codes should be very easy.
2. The application has no control over package names. If the names conflict, the code throws an error.

I'm interested in people's thoughts.

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

[codecov]

Codecov Report

Merging #425 (b2b5471) into develop (a8505cf) will increase coverage by 0.02%.
The diff coverage is 85.10%.

@@             Coverage Diff             @@
##           develop     #425      +/-   ##
===========================================
+ Coverage    57.66%   57.68%   +0.02%     
===========================================
  Files          109      109              
  Lines        11378    11387       +9     
===========================================
+ Hits          6561     6569       +8     
- Misses        4817     4818       +1     

Impacted Files	Coverage Δ
example/face_fields/face_fields_example.cpp	0.00% <0.00%> (ø)
example/particles/particles.cpp	0.00% <0.00%> (ø)
src/interface/state_descriptor.hpp	95.77% <87.50%> (-1.06%)	⬇️
example/advection/advection_driver.cpp	98.92% <100.00%> (ø)
example/advection/advection_package.cpp	98.82% <100.00%> (ø)
example/advection/parthenon_app_inputs.cpp	93.10% <100.00%> (ø)
example/calculate_pi/calculate_pi.cpp	100.00% <100.00%> (ø)
example/calculate_pi/pi_driver.cpp	92.98% <100.00%> (ø)
src/interface/state_descriptor.cpp	84.26% <100.00%> (+0.08%)	⬆️
src/interface/update.hpp	100.00% <100.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8505cf...b2b5471. Read the comment docs.

[brryan]

This is a nice improvement IMO. As a downstream consumer I think the API-breaking is justified.

It seems to me right now (and like @AndrewGaspar says) that it would be better to just throw an error if you try to add a package that's already added, I'm having a hard time imagining a scenario where you would want the application to proceed when you try to add some package that doesn't get added because of a name conflict.

One issue I worry about though is if you are trying to add two packages that have the same StateDescriptor label. What is the right thing to do here? Maybe add the second package with a mangled label (e.g. package_1), have Add return that new label, and set an internal boolean array inside Packages_t that says that there's a mismatch between the StateDescriptor label and the Packages_t key?

[Yurlungur]

Thanks, @AndrewGaspar and @brryan for your comments! I've made the changes you suggest, @AndrewGaspar except for deleting the commented out operator[] as I want to hear from the Athena-PK folks.

One issue I worry about though is if you are trying to add two packages that have the same StateDescriptor label. What is the right thing to do here? Maybe add the second package with a mangled label (e.g. package_1), have Add return that new label, and set an internal boolean array inside Packages_t that says that there's a mismatch between the StateDescriptor label and the Packages_t key?

I think for now we should just forbid this, as @AndrewGaspar suggested and throw an error. But I agree in the future we might need to be more clever.

[Yurlungur]

@AndrewGaspar I think all your review comments should be addressed now.

[jdolence]

Thanks for doing this @Yurlungur. This issue has bothered me for a long time and your resolution seems quite reasonable.

[pgrete]

Thanks for taking care of this!

I was just about to ask if the documentation was updated and then double checked to realize that we don't have docs for this piece (except for "look at the advection example") 😄
I think the pointer to the advection example is fine/enough so let's get this in.