Unify Containers and MeshBlockPacks #328

jdolence · 2020-10-21T05:48:07Z

PR Summary

Here's a stab at what was discussed for generalizing the idea of Containers (now MeshBlockData) to the Mesh level with the MeshData class.

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

…rete/meshblock-pack-in-one

pgrete · 2020-11-02T15:58:41Z

@pgrete Sorry about the particles stuff. I think the interaction between particles and meshblockpacks is going to be pretty strong (one might want only one swarm per meshblockpack, for example).

Yes, I think so too, which is why I brought this up.
A "blind" (in terms of not know how you work with this downstream) merge and conflict fix from me may have caused additional work on your side.

The whole notion of a swarm_container is probably going away but that's a nontrivial change... do you want me to try my hand at merging develop in to this PR and just seeing whether I can hack the particles implementation to still work for now?

It'd be great if you could work on the integration of develop in here. In terms of easier review (e.g., to see how the particle/swarm infrastructure is affected) it'd be great if you could work on a separate branch, i.e., derive from this one, merge develop into the new one, and then open a PR to merge the new one against this one.
I'll be able to provide feedback on that PR on short notice so that we can get that quickly in here and then merge this into develop.
This will also make it easier to add additional updates here (which I'm currently working on and that are unrelated to the swarm/particles).

…pack_particles

docs/interface/containers.md

src/interface/data_collection.hpp

src/interface/meshblock_data.hpp

src/mesh/mesh.cpp

src/mesh/meshblock.hpp

Co-authored-by: Andrew Gaspar <[email protected]>

…k_particles

Merge develop (especially particles) into meshblockpack work

pgrete · 2020-11-05T16:00:44Z

@jdolence @jdolence @AndrewGaspar after having stared at this for some more time I now suggest to make a final review of the changes that were introduced to address the comments and leave the update of the CheckRefinement and FillDerived functions for separate PRs.
My main reason is that those changes are likely more involved than we hoped for, e.g.,

CheckRefinement will require either a reduction or an Kokkos::View for the MeshBlockPacks that stored the results for each individual MeshBlock
FillDerived handles the PreFill FillDerived and PostFill functions in different ways (in the same function call to support multiple packages) that is not immediate obvious to me any more why we decided on that approach. So I'd like to discuss this in a separate issue first before making changes and then review them in a PR that (hopefully) has less than 70 files touched and 1000+ lines of code changed.

What do you think?

AndrewGaspar · 2020-11-05T16:11:03Z

FillDerived handles the PreFill FillDerived and PostFill functions in different ways (in the same function call to support multiple packages) that is not immediate obvious to me any more why we decided on that approach.

I actually don't fully appreciate why we need PreFill and PostFill. Is it so packages can have dependencies on the FillDerived of other packages?

AndrewGaspar · 2020-11-05T16:11:27Z

Actually, it can't be that, because PreFill and PostFIll aren't per-package, as pointed out.

Yurlungur · 2020-11-05T17:29:30Z

I can take another pass through if you like. But from my point of view, let's get this in and fix incremental things still missing later.

Yurlungur

I'm happy with this. As soon as the long regression tests pass, I'll pull the trigger.

Yurlungur · 2020-11-05T17:34:29Z

docs/interface/containers.md

+  // MeshBlockData of the previous stage of blocks in partition i
+  auto &mc0 = pmesh->mesh_data.GetOrAdd(stage_name[stage - 1], i);
+  // MeshBlockData of the current stage of blocks in partition i
+  auto &mc1 = pmesh->mesh_data.GetOrAdd(stage_name[stage], i);


I like this.

Yurlungur · 2020-11-05T17:39:28Z

example/advection/advection_driver.cpp

+    auto &mbase = pmesh->mesh_data.GetOrAdd("base", i);
+    auto &mc0 = pmesh->mesh_data.GetOrAdd(stage_name[stage - 1], i);
+    auto &mc1 = pmesh->mesh_data.GetOrAdd(stage_name[stage], i);
+    auto &mdudt = pmesh->mesh_data.GetOrAdd("dUdt", i);


much improved compared to the more manual method before.

Yurlungur · 2020-11-05T17:40:58Z

example/advection/advection_driver.cpp

+    // estimate next time step
+    if (stage == integrator->nstages) {
+      auto new_dt = tl.AddTask(
          fill_derived,
-          [](MeshBlock *pmb) {
-            pmb->pmr->CheckRefinementCondition();
+          [](std::shared_ptr<MeshBlockData<Real>> &rc) {
+            auto pmb = rc->GetBlockPointer();
+            pmb->SetBlockTimestep(parthenon::Update::EstimateTimestep(rc));
            return TaskStatus::complete;
          },
-          pmb);
+          sc1);
+
+      // Update refinement
+      if (pmesh->adaptive) {
+        auto tag_refine = tl.AddTask(
+            fill_derived,
+            [](std::shared_ptr<MeshBlock> pmb) {
+              pmb->pmr->CheckRefinementCondition();
+              return TaskStatus::complete;
+            },
+            pmb);
+      }


In a future MR, we should try to make these work over the whole MeshBlockPack as there is probably some advantage to doing so.

I think that's a topic for a future MR, however.

Yes, that's another item on the list of things to be addressed in a separate MR (sooner than later) as it'll make reviewing easier.

example/calculate_pi/pi_driver.cpp

Yurlungur · 2020-11-05T17:43:41Z

src/bvals/bvals_interfaces.hpp

@@ -266,6 +266,7 @@ class BoundaryVariable : public BoundaryCommunication, public BoundaryBuffer {
  bool ReceiveBoundaryBuffers() override;
  void ReceiveAndSetBoundariesWithWait() override;
  void SetBoundaries() override;
+  auto GetPBdVar() { return &bd_var_; }


Yurlungur · 2020-11-05T17:46:31Z

src/interface/data_collection.cpp

+  const std::string label = mbd_label + "_part-" + std::to_string(partition_id);
+  auto it = containers_.find(label);
+  if (it == containers_.end()) {
+    // TODO(someone) add caching of partitions to Mesh at some point


Do we know if this is a performance bottleneck? Partitioning should be very fast, as it just moves some shared pointers around. Also if packs and MeshData<Real>> are already cached, then will the partition caching ever be relevant?

As far as I've looked at the profiling data (without looking at this part specifically) this is now likely less of an issue.
I only added the comment as in the first iteration in this PR without the GetOrAdd function, the partitioning would happen in every (sub)cycle just to extract the right (cached) container.
With the current automated caching in place this should be negligible compared to other piece that are triggered when the caches are invalid (such as after load balancing).

Yurlungur · 2020-11-05T17:51:26Z

src/interface/data_collection.hpp

+    return it->second;
+  }
+
+  std::shared_ptr<T> &GetOrAdd(const std::string &mbd_label, const int &partition_id);


I feel like GetOrAdd could be unified with Get/Add a bit better. For example, perhaps there could be a version with signature

std::shared_ptr<T> &GetOrAdd(const std::string &mbd_label, const std::string &base);

to replace separate Get and Add calls elsewhere in the code.

Also perhaps the signature for the one with an int partition id should have a default for the case of only 1 partition. e.g.,

Suggested change

std::shared_ptr<T> &GetOrAdd(const std::string &mbd_label, const int &partition_id);

std::shared_ptr<T> &GetOrAdd(const std::string &mbd_label, const int partition_id = 0);

This can all be saved for another time (or never) though.

The main reason why I didn't go a more "unified" version as the GetOrAdd is specific to the DataCollection<MeshData> and not implemented for the DataCollection<MeshBlockData> (both implementation would be distinct whereas the high level GetOrAdd work in their templated form.

Yurlungur · 2020-11-05T17:53:04Z

src/interface/mesh_data.hpp

+  template <typename... Args>
+  auto PackVariables(Args &&... args) {
+    auto pack_function = [&](std::shared_ptr<MeshBlockData<T>> meshblock_data) {
+      return meshblock_data->PackVariables(std::forward<Args>(args)...);
+    };
+    std::vector<std::string> key;
+    auto vpack = block_data_[0]->PackVariables(std::forward<Args>(args)..., key);
+    return pack_on_mesh_impl::PackOnMesh<VariablePack<T>>(key, varPackMap_, block_data_,
+                                                          pack_function);
+  }
+
+  template <typename... Args>
+  auto PackVariablesAndFluxes(Args &&... args) {
+    auto pack_function = [&](std::shared_ptr<MeshBlockData<T>> meshblock_data) {
+      return meshblock_data->PackVariablesAndFluxes(std::forward<Args>(args)...);
+    };
+    vpack_types::StringPair key;
+    auto vpack = block_data_[0]->PackVariablesAndFluxes(std::forward<Args>(args)..., key);
+    return pack_on_mesh_impl::PackOnMesh<VariableFluxPack<T>>(key, varFluxPackMap_,
+                                                              block_data_, pack_function);
+  }


src/interface/meshblock_data.cpp

Yurlungur · 2020-11-05T19:04:01Z

Tests pass. Pulling the trigger.

pgrete and others added 30 commits September 4, 2020 10:50

Rename MakeTasks to MakeTaskCollection

57fd293

Use TaskColletion in Advection example

9f472ea

Use MeshTask for FluxDivergence

f032651

Fix format and sparse fix

cd61c7d

Pass blocks as reference

9ca7943

Update and Average to MeshBlockPacks

9f674c8

WIP pack-in-one buffers

7c97d53

SetBoundaries pack in one

4b049fa

Fix ptr access

6f3379a

Merge branch 'develop' into pgrete/meshblock-pack-in-one

6a0d8b5

Update interfaces

f5f0b2f

Merge branch 'jmm/meshblockpack-cache' into pgrete/meshblock-pack-in-one

6f3c07f

Fix interfaces from merge

985c958

Fix kernel naming in advection package

328652f

Move buffer pack in one to separate file

23a1856

Add profile markers to boundary funcs

3afdc10

Merge remote-tracking branch 'origin/jmm/meshblockpack-cache' into pg…

9f8fc14

…rete/meshblock-pack-in-one

Reintroduce container based update func

80e21e5

FluxDiv uses BlockPacks

f347072

Add ndim_ to MeshBlockPack to make avail on host

20253e2

Update and Average use MeshBlockPacks

7126d6f

Name refactor

c34d7ff

Reuse packs in Send/SetBoundaries

ae6aa03

Merge branch 'develop' into pgrete/meshblock-pack-in-one

d586a9b

Use pack caching in Advection example

0fb9b23

Fix task dependency

90ef006

fix package name

c48455e

Container -> MeshBlockData

e9dc001

changing more type names and file names

d944890

generalize MeshBlockDataCollection to DataCollection

1205167

brryan added 3 commits November 2, 2020 12:03

Merge remote-tracking branch 'upstream/develop' into brryan/meshblock…

44910bb

…pack_particles

Fix up particles example

edf6041

Formatting

ca62f86

brryan mentioned this pull request Nov 2, 2020

Merge develop (especially particles) into meshblockpack work #346

Merged

5 tasks

AndrewGaspar suggested changes Nov 2, 2020

View reviewed changes

pgrete and others added 12 commits November 3, 2020 18:39

Apply suggestions from @AndrewGaspar review

ab28021

Co-authored-by: Andrew Gaspar <[email protected]>

Addressing more review comments

714d673

Merge branch 'develop' into brryan/meshblockpack_particles

b945d8f

Merge branch 'jdolence/containers_and_packs' into brryan/meshblockpac…

cd3ad1e

…k_particles

Use templated DataCollection for SwarmData

0d8c61d

Remove rogue header include

af14d66

Reintroduce pack helper function (now with cache support)

25fd028

Merge pull request #346 from lanl/brryan/meshblockpack_particles

afc8cc5

Merge develop (especially particles) into meshblockpack work

Merge branch 'develop' into jdolence/containers_and_packs

e6468fd

Merge branch 'develop' into jdolence/containers_and_packs

5473532

Update Changelog

fb57ae7

Add more doc

ebb7735

pgrete changed the title ~~[WIP] Unify Containers and MeshBlockPacks~~ Unify Containers and MeshBlockPacks Nov 5, 2020

AndrewGaspar approved these changes Nov 5, 2020

View reviewed changes

pgrete self-requested a review November 5, 2020 16:05

pgrete approved these changes Nov 5, 2020

View reviewed changes

Yurlungur approved these changes Nov 5, 2020

View reviewed changes

Yurlungur merged commit f47690f into develop Nov 5, 2020

pgrete deleted the jdolence/containers_and_packs branch November 6, 2020 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify Containers and MeshBlockPacks #328

Unify Containers and MeshBlockPacks #328

jdolence commented Oct 21, 2020 •

edited by pgrete

Loading

pgrete commented Nov 2, 2020

pgrete commented Nov 5, 2020

AndrewGaspar commented Nov 5, 2020

AndrewGaspar commented Nov 5, 2020

Yurlungur commented Nov 5, 2020

Yurlungur left a comment

Yurlungur Nov 5, 2020

Yurlungur Nov 5, 2020

Yurlungur Nov 5, 2020

pgrete Nov 5, 2020

Yurlungur Nov 5, 2020

Yurlungur Nov 5, 2020

pgrete Nov 5, 2020

Yurlungur Nov 5, 2020

pgrete Nov 5, 2020

Yurlungur Nov 5, 2020

Yurlungur commented Nov 5, 2020

	std::shared_ptr<T> &GetOrAdd(const std::string &mbd_label, const int &partition_id);
	std::shared_ptr<T> &GetOrAdd(const std::string &mbd_label, const int partition_id = 0);

Unify Containers and MeshBlockPacks #328

Unify Containers and MeshBlockPacks #328

Conversation

jdolence commented Oct 21, 2020 • edited by pgrete Loading

PR Summary

PR Checklist

pgrete commented Nov 2, 2020

pgrete commented Nov 5, 2020

AndrewGaspar commented Nov 5, 2020

AndrewGaspar commented Nov 5, 2020

Yurlungur commented Nov 5, 2020

Yurlungur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yurlungur commented Nov 5, 2020

jdolence commented Oct 21, 2020 •

edited by pgrete

Loading