Add support for uint64 swarm vars and add default ids by pgrete · Pull Request #1253 · parthenon-hpc-lab/parthenon

pgrete · 2025-05-19T16:20:37Z

PR Summary

What it says on the tin.

I was about to add some logic to enforce globally unique ids (which can be added dynamically) based on MPI logic and then thought about a simpler way(which kind of requires a larger pool of numbers, i.e., uint64_t without being too restrictive.
Thus, I added support for that data type first.

When we add another data type to particles, we probably want to start working with additional template magic as I was touching way more code than anticipated.

As it stands right now, the new swarm.id field is added by default for all Swarms.
Is this what we want? I kind of lean towards it as it adds little overhead and potentially solves some hassle down the road.
UPDATE: Seems that we want it. The old behavior (i.e., no id field) be recovered by adding a swarm with the Metadata::NoPersistentParticleIds flag

Another item, I'm currently unsure about is the id field for postprocessing tools (like visit or Paraview).
Do those tools require a field named id (rather than swarm.id or sth else)? We could add an override to the xdmf output.
UPDATE: A default "id" field (with this exact name) is now always added to the xdmf file either pointing to the real unique ids or some automatically generated field (only existing in the output) as before

Last big question: What do we do about backwards compatibility? I imagine this could get a little bit ugly given that the underlying datatype changed.
UPDATE: Should not be an issue any more by adding Metadata::NoPersistentParticleIds to the swarm.

Anything else I missed?

Breaking Changes

A unique particle id field is added by default to each swarm.
If this is not desired (and to recover the old behavior), the field can be disabled by passing the Metadata::NoPersistentParticleIdsflag.

PR Checklist

Yurlungur · 2025-05-19T19:08:48Z

As it stands right now, the new swarm.id field is added by default for all Swarms.
Is this what we want? I kind of lean towards it as it adds little overhead and potentially solves some hassle down the road.

I don't think it hurts much, but may be hard to interpret in some cases, e.g., what about Monte Carlo packets, where many may be created and destroyed over the course of a simulation? Peristent IDs for these may be a pain. I say it's fine to always make them exist by default, but I would want some warning labels in the docs/comments. Others should chime in. @pdmullen?

Another item, I'm currently unsure about is the id field for postprocessing tools (like visit or Paraview).
Do those tools require a field named id (rather than swarm.id or sth else)? We could add an override to the xdmf output.

The field can be named whatever we like so long as the XML calls it id and points at it appropriately. But honestly, I never got the XDMF working for particles. I still don't understand why.

Yurlungur

LGTM. Though we may want to keep the ability to automatically set the IDs to the global ID in output somehow?

pdmullen · 2025-05-19T19:24:33Z

As it stands right now, the new swarm.id field is added by default for all Swarms.
Is this what we want? I kind of lean towards it as it adds little overhead and potentially solves some hassle down the road.

I don't think it hurts much, but may be hard to interpret in some cases, e.g., what about Monte Carlo packets, where many may be created and destroyed over the course of a simulation? Peristent IDs for these may be a pain. I say it's fine to always make them exist by default, but I would want some warning labels in the docs/comments. Others should chime in. @pdmullen?

Another item, I'm currently unsure about is the id field for postprocessing tools (like visit or Paraview).
Do those tools require a field named id (rather than swarm.id or sth else)? We could add an override to the xdmf output.

The field can be named whatever we like so long as the XML calls it id and points at it appropriately. But honestly, I never got the XDMF working for particles. I still don't understand why.

I'd echo @Yurlungur's thoughts. For Monte Carlo, we are constantly destroying and creating new particles... The gears are still spinning in my head re: if there are any edge cases where the current changeset leads to issues given the above. Meanwhile, I'd also like to test this changeset against our downstream code jaybenne.

Also, XDMF outputs do now work for swarms... I added this functionality when SwarmPacks were introduced. There is already a check in there for an ID field:

https://github.com/parthenon-hpc-lab/parthenon/blob/develop/src/outputs/parthenon_xdmf.cpp#L252

We might want to change occurences of "id" to id::name()

Yurlungur · 2025-05-19T19:33:34Z

Also, XDMF outputs do now work for swarms... I added this functionality when SwarmPacks were introduced. There is already a check in there for an ID field:

They do??? What was I doing wrong before? Have you tried this with both visit and paraview?

pdmullen · 2025-05-19T19:36:15Z

Also, XDMF outputs do now work for swarms... I added this functionality when SwarmPacks were introduced. There is already a check in there for an ID field:

They do??? What was I doing wrong before? Have you tried this with both visit and paraview?

#1079
I had only tested with VisIt at that time.

Yurlungur · 2025-05-19T19:39:48Z

Also, XDMF outputs do now work for swarms... I added this functionality when SwarmPacks were introduced. There is already a check in there for an ID field:

They do??? What was I doing wrong before? Have you tried this with both visit and paraview?

#1079 I had only tested with VisIt at that time.

I completely forgot about this lol. Thanks again for finding the magic incantation that worked.

…ompi

pgrete · 2025-05-21T13:50:06Z

Alright, following the discussion I made some updates to the PR.

I added a NoPersistentParticleIds Metadata flag
Adding said flag to particles reverts to the old default behavior (i.e., no "additional" id field)
Reintroduced writing default "id"s if the internal field has been disabled
Overwrite the internal id variable name in the xdmf file with the common "id" one.
Updated the docs to reflect those changes
Using the logic in the particle tracer example for testing

pgrete · 2025-05-21T13:55:30Z

+        // TODO(reviewers) Why are do we forcibly disallow writing particle xdmf for
+        // restarts?
        op.write_swarm_xdmf =
            (restart) ? false


Any reason for doing this? I'd like to be able to generate particle xdmf for restart outputs (as those are sometime the only type I write).

@pdmullen ? My guess is it's fine to write particle xdmf for restarts but that since it's an extra file it's just a lot of output?

You have my blessing.

If I had to guess (it has been a while...), I disabled this because I wanted the imprint of my swarm XDMF MR to be minimal since it requires a bunch of ugly geometry writes. That code is so ugly (but the only path I could find to make VisIt happy...)

I was not suggesting to enable it by default I was suggest to allow enabling it.
Right now the write_swarm_xdmf parameter is ignored for restart outputs (i.e., it's impossible to write particle xdmf for restart files.

All good by me :)

pgrete · 2025-06-02T14:37:28Z

@mfournier01 discovered and correctly isolated a casting issue for the uint64 type (as we're always communicating packed Reals).

With the latest commit I now use a std::memcpy and added static asserts for int and uint64 in the packing and unpacking routines.

@pdmullen were you able to test downstream and/or are there any other changes required?

pdmullen · 2025-06-02T14:46:54Z

@mfournier01 discovered and correctly isolated a casting issue for the uint64 type (as we're always communicating packed Reals).

With the latest commit I now use a std::memcpy and added static asserts for int and uint64 in the packing and unpacking routines.

@pdmullen were you able to test downstream and/or are there any other changes required?

Thanks for the bug fix (i.e., thanks @mfournier01 !) @pgrete, I'll have an answer for you re: downstream by no later than tomorrow COB.

pdmullen · 2025-06-04T15:19:24Z

@mfournier01 discovered and correctly isolated a casting issue for the uint64 type (as we're always communicating packed Reals).
With the latest commit I now use a std::memcpy and added static asserts for int and uint64 in the packing and unpacking routines.
@pdmullen were you able to test downstream and/or are there any other changes required?

Thanks for the bug fix (i.e., thanks @mfournier01 !) @pgrete, I'll have an answer for you re: downstream by no later than tomorrow COB.

Sorry for the delay on this... something pertinent came up.

lroberts36

LGTM

lroberts36 · 2025-06-06T02:27:58Z

+   general. Therefore, if swarms do not automatically contain a unique ID swarm
+   variable (because the ``Metadata::NoPersistentParticleIds`` has been passed when
+   creating the swarm), Parthenon will generate an ``id`` variable just for output
+   (i.e., it is still not available within the simulation itself) and write it to file.


Does the use still have to request that id be output?

lroberts36 · 2025-06-06T02:30:32Z

  std::string swarm_name = "tracers";
-  Metadata swarm_metadata({Metadata::Provides, Metadata::None});
+  Metadata swarm_metadata(
+      {Metadata::Provides, Metadata::None, Metadata::NoPersistentParticleIds});


Why is the id field being removed here when previously the example explicitly created an id field?

I made this change to test the Metadata::NoPersistentParticleIds feature.
The particle_tracer regression test results in particles being in the same order, i.e., the automatically generated ids match, and it also tests accessing "id" via that string rather than swarm.id.
Given that this might be confusing I now added comments both here and in the test.

lroberts36 · 2025-06-06T02:42:27Z

+            // Always output id, x, y, and z for swarms so that they work with vis tools.
+            // Note, it's fine to add the id by default (even though it might not actually
+            // exist) because only variables that do exists are actually being written.
+            std::vector<std::string> coords = {
+                swarm_position::id::name(), swarm_position::x::name(),
+                swarm_position::y::name(), swarm_position::z::name()};


This looks like it answers my question above.

* Add support for uint64 swarm vars and enforce ids * Provide known name for id field in xdmf. Always write ids * Allow disabling unique ids for particles (old default behavior) * Update Changelog * Fix doc format * Allow writing of swarm xdmf for rst outputs * Fix unsafe/errornous casting * Add comment on use of nonpersistnet ids in traces

* start * seemingly works * format * remove intermediate prints * print some solutions * small * remove comments * Increase MPI timeout counter by 10x (#1243) * cleanup output and add option of outputting a specific stage * docs * changelog and formatting * oops swarms are only in Base * formatting * lukes nicer cleanup * add compute asymmetry script * Add shebang to movie2d * document how to change output cadence from restart * changelog * CC * CC * add discussion of SMR to AMR doc * changelog * typo thanks patrick * Try to move all LogicalLocation related logic to class methods * fix bugs * make sure colorbar label generated * add resport_asymmetry * Update doc/sphinx/src/outputs.rst Co-authored-by: Philipp Grete <pgrete@hs.uni-hamburg.de> * Update doc/sphinx/src/outputs.rst Co-authored-by: Philipp Grete <pgrete@hs.uni-hamburg.de> * Add checks for `parthenon/mesh/multigrid = true` when using multigrid block lists (#1241) * add checks and move things to being private * changelog * [Bugfix]: Propagate flux `Metadata` correctly in `SparsePool` (#1249) * propagate flux metadata correctly in SparsePool * changelog * [Trivial] Fix leftover reference to old coords interface (#1251) * Fix leftover reference to old coords interface * Fix doc formatting * Release 25.05 (#1252) * Release 25.05 * Update CHANGELOG.md * Add support for uint64 swarm vars and add default ids (#1253) * Add support for uint64 swarm vars and enforce ids * Provide known name for id field in xdmf. Always write ids * Allow disabling unique ids for particles (old default behavior) * Update Changelog * Fix doc format * Allow writing of swarm xdmf for rst outputs * Fix unsafe/errornous casting * Add comment on use of nonpersistnet ids in traces * New CUDA CI+development Docker container (#1162) * update CI Docker container Update to CUDA 12.6, Ubuntu 24.04, and Clang 19. * update OpenMPI * revert to OpenMPI 4.1.4 * add ADIOS2+openPMD * add c-blosc ubuntu package * fix Dockerfile.nvcc * fix dockerfile * install python headers * install cmake from apt (required for aarch64) * remove duplicate cmake dep * disable ascent build * add newer ascent version * disable ascent build; fix openpmd build * downgrade to CUDA 12.0 * fix ascent build path * fix bug in build_ascent.sh * remove unneeded patches * ascent complains if MFEM is not built * control cuda support for ascent with env var * add MAKEOPTS=--output-sync=target * add comment to Dockerfile * Downgrade numpy * Fix ADIOS2 and OpenPMD versions * Directly use Ascent script with small patch * Use Cuda12.1 container and drop to local user * add emacs and vi * set build_jobs=`nproc` to avoid OOM kill * add developer tools for Codespaces/VSCode * add devcontainer.json * update to CUDA 12.8 and VTK-m 2.3 * update image ref * fetch BLT * extract BLT into correct dir * avoid uid 1000 * build and publish Docker image based on Dockerfiles in repo * Update CI image to be used * Fix python version used for linting * Bump opmd to stable release * Add changelog * Use updated clang for compiler check * remove docker-publish action * Fix C++20 build * Attempt to fix Parthenon Ascent dep * Try Ben's BLT build fix * Fix Ascent build * Include opmd in rocm image * Use actual image --------- Co-authored-by: Philipp Grete <pgrete@hs.uni-hamburg.de> * Fix -m (#1257) * Exit cleanly if -m is set after Mesh is constructed but before initialize * Update CHANGELOG.md * remove kokkos warning * add core dump output format * check that only one core dump file is requested * changelog * update docs and delete accidentally added files * output postfix -> .chdf * make sure to slurp in fluxes * add a 5 * CC * more CC * example CC * comment that ghost zones exist * seemingly working timeout capability * seemingly working * add timing option * format * changelog * Remove old receive try test * remove comm buffer hang check * use task collection for internal mesh communication * Add timeout parameter to doc * format * Update src/mesh/mesh.cpp * Update src/tasks/tasks.hpp * Fix auto-docs and typo * respond to Philipp's comments * check the task collection return status --------- Co-authored-by: Philipp Grete <pgrete@hs.uni-hamburg.de> Co-authored-by: Jonah Miller <jonahm@lanl.gov> Co-authored-by: Jonah Miller <jonah.maxwell.miller@gmail.com> Co-authored-by: Ben Wibking <ben@wibking.com> Co-authored-by: Adam M. Dempsey <adempsey@lanl.gov>

Add support for uint64 swarm vars and enforce ids

687ce9b

pgrete requested review from Yurlungur, adamdempsey90, c-prather, lroberts36 and pdmullen May 19, 2025 16:20

Yurlungur approved these changes May 19, 2025

View reviewed changes

pgrete added 4 commits May 21, 2025 12:34

Provide known name for id field in xdmf. Always write ids

6dc5f50

Allow disabling unique ids for particles (old default behavior)

4cdd285

Merge remote-tracking branch 'origin/develop' into pgrete/swarm-uid-n…

0b58e65

…ompi

Update Changelog

1c1d524

pgrete changed the title ~~Add support for uint64 swarm vars and enforce ids~~ Add support for uint64 swarm vars and add default ids May 21, 2025

pgrete added the breaks-downstream label May 21, 2025

pgrete commented May 21, 2025

View reviewed changes

pgrete added 2 commits May 21, 2025 16:01

Fix doc format

4bf23a7

Allow writing of swarm xdmf for rst outputs

4e3d65b

pgrete enabled auto-merge (squash) May 22, 2025 08:28

pgrete disabled auto-merge May 22, 2025 08:28

Fix unsafe/errornous casting

c535fb0

pgrete enabled auto-merge (squash) June 2, 2025 14:35

pgrete disabled auto-merge June 2, 2025 14:35

lroberts36 approved these changes Jun 6, 2025

View reviewed changes

pgrete added 2 commits June 6, 2025 11:11

Merge branch 'develop' into pgrete/swarm-uid-nompi

bae2800

Add comment on use of nonpersistnet ids in traces

61f35da

pgrete enabled auto-merge (squash) June 6, 2025 09:33

pgrete merged commit 1671c57 into develop Jun 6, 2025
35 checks passed

pgrete deleted the pgrete/swarm-uid-nompi branch June 6, 2025 12:33

pdmullen mentioned this pull request Mar 25, 2026

Add uint64 support to SwarmPacks #1375

Merged

13 tasks

Conversation

pgrete commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Breaking Changes

PR Checklist

Uh oh!

Yurlungur commented May 19, 2025

Uh oh!

Yurlungur left a comment

Choose a reason for hiding this comment

Uh oh!

pdmullen commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yurlungur commented May 19, 2025

Uh oh!

pdmullen commented May 19, 2025

Uh oh!

Yurlungur commented May 19, 2025

Uh oh!

pgrete commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgrete commented Jun 2, 2025

Uh oh!

pdmullen commented Jun 2, 2025

Uh oh!

pdmullen commented Jun 4, 2025

Uh oh!

lroberts36 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pgrete commented May 19, 2025 •

edited

Loading

pdmullen commented May 19, 2025 •

edited

Loading