Make fluxes separate variables

[lroberts36]

PR Summary

Following # #764, this PR promotes fluxes to be their own variables and then implements flux correction through the regular boundary communication machinery. Now when a variable has the Metadata::WithFluxes flag set, a new variable is created with the flags Metadata::Flux and Metadata::OneCopy and the name bnd_flux:: + original var name (the prepending is required because of issue #1032). This variable is packed transparently so that packs that are created with fluxes pack associated flux variables automatically and calls to pack.flux(...) are unchanged. As a result, this PR should have no impact on downstream codes.

Notes:

• Allows "fluxes" (in the generalized Stokes theorem sense) for non-cell centered fields. This is fully implemented but untested.
• Removes all of the old face flux correction code paths that had to be maintained separately from the boundary values code.
• Should communicate the same information as was communicated by the previous flux correction routines, i.e. only shared elements are communicated on fine to coarse boundaries, not the entire ghost halo on all boundaries.
• This may not produce bitwise identical results to the old code, since the order in which field elements are summed during restriction may not be the same as the order that the old flux correction routines did the restriction sums in. Nevertheless, any deviation beyond machine precision is not expected.
• The flux fields have the same sizes as their parent fields (i.e. the shape of a face flux field in the tuvkji directions is the same as that of the a cell centered field). This is to be consistent with the previous shape of flux fields and to allow for flattened loops that go over more than just the i indices of a field (as is commonly done in Riot for instance).

PR Checklist

• [Small] Cleanup of NeighborBlock #1042 is merged into develop
• Code passes cpplint
• 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

[bprather]

Ah, fair enough, I didn't consider that the memory would be the same for face field/edge flux variables anyway.

Re: ghosts, sometimes KHARMA fills EMFs to the last ghost edges of domain boundaries, and expects a consistent face value, for e.g. preparing Dirichlet/frozen boundary conditions. But, this should be fine since the edge and face variables will both be (N+1)^3.

I'm not aware of anyone requiring the last face value of any face-centered variable which could conceivably be marked a flux. So I guess the concern is purely aesthetic, and in any case non-blocking.

[lroberts36]

@Yurlungur: Some documentation added/fixed to be consistent.

[lroberts36]

The issue with the riot tests mentioned during the Parthenon call today has been figured out. It had to do with a kernel grabbing all variables in a MeshData object and zeroing them out.

Because fluxes are now variables, a change to how MeshData and MeshBlockData are built when DataCollection::Add(const std::string &name, const std::shared_ptr<T> &src, const std::vector<ID_t> &fields, const bool shallow) is called was required for backward compatibility. Rather than just add the variables contained in fields, we now add those variables and the flux variables associated with them. This ensures that when packs are created from these MeshData and PDOpt::WithFluxes is requested, the fluxes are in fact available to pack.

The issue in Riot was that a MeshData object was created through DataCollection::Add, then a pack was created from this MeshData using variable_names::any, and then all of the fields were zeroed before they were accumulated into. This meant that the one-copy fluxes were zeroed during this call since they were automatically included in the MeshData and then sucked up by any. The solution chosen to fix this was to make variable_names::any only select variables that didn't include bnd_flux:: in their name. I am not sure this is the best way to deal with it, but that should fix things for now.

[pgrete]

Nice cleanup!
I just got a couple of interface/doc comments -- nothing big.

[pgrete]

This should be Get(0,0,0) doesn't it?
And if so, what's this test doing if it passes when using the wrong fluxes?

Separately, (along with @Yurlungur similar comment on Get below), what about a more intuitive interface like
GetFlux(varname, direction) that hides the flux name and extra 0 indices?

[lroberts36]

Yes, it is disturbing that this test passes with that bug. I am not really sure what it is supposed to be doing though...

The Get function called here is really ParArrayGeneric::Get(Args... args), so I don't think we can reasonably make a flux based overload. I don't think we really need to support it from MeshBlockData either, since I don't think we want to encourage the pattern that is used here (variables should be accessed through packs rather than pulling out raw ParArrays directly from MeshBlockData). That being said, I agree that the ParArrayGeneric::Get interface is a bit confusing and should possibly be changed. That is for another time though I think...

[pgrete]

I see how this makes the code path in the driver cleaner (wrt to dependencies) but somehow passing a variable in and just use it to immediately return feels odd (just commenting, not saying this should change).

[lroberts36]

I tend to like this better than not taking multilevel as an argument and having people include

auto flx_cor = prev_task;
if (multilevel) {
  flx_cor = AddFluxCorrectionTasks(flx_cor, ...); 
}

[pgrete]

The :: may or may not cause a problem as it's adisallowed character by the OpenPMD standard.
I'm actually not sure if this is a hard problem or not (as I assume that some default tools may not work with that approach, but we probably would dump these flux variable only for debugging anyway, don't we)?
Also, we could in principle overwrite the reserved pattern for output and do a name matching when reading them back in again.

[pgrete]

May that have some unintended side-effects?

[lroberts36]

I don't think so, since there are few places where any is currently used and those places assumed that fluxes would not get sucked up when using any. I think in the future we, should avoid using any and use either any_nonautoflux or any_withautoflux.