Updates to `DataCollection` #1092

lroberts36 · 2024-05-29T05:14:48Z

PR Summary

This PR makes some changes to DataCollection so that calling pmesh->mesh_data.GetOrAdd(some_label, partition) works whether or not some_label has been previously added to the MeshBlockData container. Additionally, it makes it so that Add will now work correctly for MeshData (previously it didn't use the correct labeling and containers could be overwritten).

Possible other things to do:

Formalize partitioning and move it to Mesh
Get all of the DataCollection interface to work with both Mesh and MeshData
Remove Initialize methods and replace them with constructors since they break RAII

PR Checklist

Yurlungur

Thanks for cleaning this up. I think these are good changes.

src/interface/meshblock_data.hpp

Yurlungur · 2024-05-29T15:35:40Z

src/interface/meshblock_data.hpp

+      if (stage_name_ == "base") {
+        const auto &swarm_container = GetSwarmData();
+        swarm_container->Initialize(resolved_packages, GetBlockSharedPointer());
+      }


Ping @pdmullen @brryan

I don't see any issue with this. Swarms are OneCopy-like and always belong to the base stage so it doesn't really make sense to Add a new MeshBlockData to the DataCollection based on an existing MeshBlockData. We have thought a bit about the best way to express this through the parthenon interface but don't have any good thoughts at the moment.

What was the concern here? Sorry I am late to the party...

We currently adopt a model wherein the "base" MeshBlockData register contains our swarm data. We have checks that swarms are only accessed through said register.

I just wanted to know if this was the desired behavior and if there were any side effects of what I did.

I'll test downstream first thing tomorrow.

Did the test happen? @pdmullen

They did, there is an old comment from Patrick below.

src/interface/data_collection.hpp

src/utils/concepts_lite.hpp

…-meshblockdata-initialization

pdmullen · 2024-05-30T14:58:54Z

w.r.t. swarms, I tested this downstream and all looks good. I noticed that this PR is targeting lroberts36/add-fine-variables instead of develop... intentional?

lroberts36 · 2024-05-30T17:18:48Z

I noticed that this PR is targeting lroberts36/add-fine-variables instead of develop... intentional?

@pdmullen: yeah, but only because I was using the new test I added there to check that removing the MeshBlockData boiler plate worked.

Yurlungur · 2024-06-10T18:09:50Z

@lroberts36 are we waiting for someone to review this?

lroberts36 · 2024-06-10T18:12:05Z

@lroberts36 are we waiting for someone to review this?

@Yurlungur: @pgrete said he would on Thursday, but I think he has been under the weather the last few days.

…ization

pgrete

LGTM

"Remove Initialize methods and replace them with constructors since they break RAII" is possibly meant for a future PR, isn't it? In that case -> new issue?

pgrete · 2024-06-14T07:23:34Z

src/interface/data_collection.hpp

+  std::string GetKey(const std::string &stage_label, std::optional<int> partition_id,
+                     std::optional<int> gmg_level) {
+    std::string key = stage_label;
+    if (partition_id) key = key + "_part-" + std::to_string(*partition_id);
+    if (gmg_level) key = key + "_gmg-" + std::to_string(*gmg_level);


Should there be a debug_require that only partition_id or gmg_level or neither is passed?

no, the GMG levels are also partitioned. Although to be fair, partitioning has always been broken for multigrid, i.e. it only works when all blocks are included in the partition. This is on my list of things to fix.

pgrete · 2024-06-14T07:31:38Z

src/interface/meshblock_data.hpp

+      if (stage_name_ == "base") {
+        const auto &swarm_container = GetSwarmData();
+        swarm_container->Initialize(resolved_packages, GetBlockSharedPointer());
+      }


Did the test happen? @pdmullen

pgrete · 2024-06-14T07:35:47Z

example/fine_advection/advection_driver.cpp

-  if (stage == 1) {
-    for (int i = 0; i < blocks.size(); i++) {
-      auto &pmb = blocks[i];
-      // first make other useful containers
-      auto &base = pmb->meshblock_data.Get();
-      pmb->meshblock_data.Add("dUdt", base);
-      for (int s = 1; s < integrator->nstages; s++)
-        pmb->meshblock_data.Add(stage_name[s], base);
-    }
-  }


Should this be removed in the other examples, too, to decrease likelihood of reusing this pattern?

Probably, but let's leave it for a future PR. I think it would be nice to work to clean up the examples in the near future.

…ization

lroberts36 · 2024-06-15T16:39:06Z

"Remove Initialize methods and replace them with constructors since they break RAII" is possibly meant for a future PR, isn't it? In that case -> new issue?

@pgrete: I think I was just annoyed when I wrote this comment. It is probably not worth the effort to remove the initialization methods and replace them with constructors.

…ization

lroberts36 added 10 commits May 28, 2024 13:07

Remove undefined constructors

32c1161

beating around the bushes...

0613951

Actually enforce that we only get this on the base stage

2880bd0

move setting grid to MeshData constructor

bbd956f

Generalize Add to take a different source

dd6498e

Remove boiler plate

3a33ff6

Move always_false to concepts_lite

8982aa6

Add type checking comment

3729dae

cleanup and make Add work for MeshData

33f6ced

format

b4facf0

Yurlungur approved these changes May 29, 2024

View reviewed changes

lroberts36 added 3 commits May 29, 2024 11:17

Actually check the key instead of the name

5909597

format

58cc64d

Add comments to static_asserts

9c53bae

lroberts36 requested review from jdolence and pgrete May 29, 2024 19:30

changelog

0fc1d55

lroberts36 requested review from pdmullen and brryan May 29, 2024 19:44

Merge branch 'lroberts36/add-fine-variables' into lroberts36/refactor…

969e56d

…-meshblockdata-initialization

lroberts36 added the bug Something isn't working label May 30, 2024

This was referenced Jun 5, 2024

[Breaking change] Add fine field option #991

Merged

Formalizing MeshBlock partitions for creating MeshData #1097

Closed

lroberts36 changed the base branch from lroberts36/add-fine-variables to develop June 10, 2024 19:44

lroberts36 changed the base branch from develop to lroberts36/add-fine-variables June 10, 2024 19:44

lroberts36 changed the base branch from lroberts36/add-fine-variables to develop June 13, 2024 18:11

Merge branch 'develop' into lroberts36/refactor-meshblockdata-initial…

cd86780

…ization

pgrete approved these changes Jun 14, 2024

View reviewed changes

Merge branch 'develop' into lroberts36/refactor-meshblockdata-initial…

04d8e1e

…ization

Merge branch 'develop' into lroberts36/refactor-meshblockdata-initial…

11324a9

…ization

lroberts36 enabled auto-merge June 15, 2024 21:12

lroberts36 merged commit 7eb6190 into develop Jun 15, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to `DataCollection` #1092

Updates to `DataCollection` #1092

lroberts36 commented May 29, 2024 •

edited by pgrete

Loading

Yurlungur left a comment

Yurlungur May 29, 2024

brryan May 29, 2024

pdmullen May 30, 2024 •

edited

Loading

lroberts36 May 30, 2024

pdmullen May 30, 2024

pgrete Jun 14, 2024

lroberts36 Jun 15, 2024

pdmullen commented May 30, 2024

lroberts36 commented May 30, 2024

Yurlungur commented Jun 10, 2024

lroberts36 commented Jun 10, 2024

pgrete left a comment

pgrete Jun 14, 2024

lroberts36 Jun 15, 2024

pgrete Jun 14, 2024

pgrete Jun 14, 2024

lroberts36 Jun 15, 2024

lroberts36 commented Jun 15, 2024

Updates to DataCollection #1092

Updates to DataCollection #1092

Conversation

lroberts36 commented May 29, 2024 • edited by pgrete Loading

PR Summary

PR Checklist

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

pdmullen May 30, 2024 • edited Loading

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

pdmullen commented May 30, 2024

lroberts36 commented May 30, 2024

Yurlungur commented Jun 10, 2024

lroberts36 commented Jun 10, 2024

pgrete 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

lroberts36 commented Jun 15, 2024

Updates to `DataCollection` #1092

Updates to `DataCollection` #1092

lroberts36 commented May 29, 2024 •

edited by pgrete

Loading

pdmullen May 30, 2024 •

edited

Loading