WIP: Towards Curvilinear Swarms - #1391
Conversation
… pdmullen/pack-refactor
Co-authored-by: Luke Roberts <lfroberts@lanl.gov>
…/parthenon into pdmullen/pack-refactor
…en/swarm-mesh-data-tasks
…en/swarm-mesh-data-tasks
Yurlungur
left a comment
There was a problem hiding this comment.
👍 Good change and good use of AI.
One possible suggestion below regarding the python API.
|
|
||
|
|
||
| ### Incompatibilities (i.e. breaking changes) | ||
| - [[PR 1391]](https://github.com/parthenon-hpc-lab/parthenon/pull/1391) Towards Curvilinear Swarms |
| def x(self): | ||
| return self.Get("swarm.x") | ||
| def x1(self): | ||
| return self.Get("swarm.x1") |
There was a problem hiding this comment.
It's really easy to keep backwards compatibility for the python with maybe a try-catch block and overloading both sets of names for these functions. I think we might want to do that because otherwise phdf won't be backwards compatible with old output/dump files, which we may want to mitigate.
There was a problem hiding this comment.
happy to do that.
|
@pdmullen will look into backwards compatibilty (especialyl for outputs) |
pgrete
left a comment
There was a problem hiding this comment.
In general, the changes look good.
Given that OpenPMD outputs are now merged, the new changes need to be also implemented for those outputs (plus keeping hte logic to choose the restart reader).
Should I take a stab at this or do you @pdmullen ?
Also should we add safeguards again in functions that will definitely result in wrong results in non-cartesian coordinates (like SortParticlesByCell)?
I'm happy to take a stab at it! |
Can you take a look at 834fdea and scream at me if I messed anything up?
AFAIK, safeguards are not needed for |
pgrete
left a comment
There was a problem hiding this comment.
I still need to test this in practice, but I'm wondering what this make to restart old sims with the new interface with regard to also keep the old naming (so that outputs for a given simulations are consistent in the name being used).
This also applies to HDF5 outputs.
Otherwise, we might want/gave to bump the output format version to reflect those changes.
| particle_record = "position"; | ||
| particle_record_component = "x"; | ||
| } else if (vname == swarm_position::y::name()) { | ||
| particle_record_component = "x1"; |
There was a problem hiding this comment.
I think we may need to be more specific wrt coordinate component naming as the standard has specific names depending on the coordinate system (x,y,z or r,t,z), see https://github.com/openPMD/openPMD-standard/blob/latest/STANDARD.md#naming-conventions and geometry parameter in https://github.com/openPMD/openPMD-standard/blob/latest/STANDARD.md#required-attributes-for-each-mesh-record
Given that some pieces are still "reserved" my suggestion is to keep x,y,z for Cartesian, default to x1,x2,x3 for non-Cartesian with a warning that the output metadata is currently non-standard compliant and that people wanting to use it in production should get in touch to discuss next steps/a proper implementation.
PR Summary
Made in part with generative AI.
Swarm positions are now treated as native mesh coordinates x1, x2, and x3 rather than Cartesian x, y, and z. That change updates the default swarm variable names, the internal swarm coordinate accesses, output defaults, and
the touched examples/tests so the swarm infrastructure is expressed in coordinate-system-native terms.
On the remeshing side, the refinement branch now classifies particles against
the actual daughter split interfaces returned by the mesh topology, expressed
in x1/x2/x3. For the current uniform coordinate systems this is equivalent to
the old midpoint logic, but it is clearer and better aligned with the
generalized coordinate interpretation. We still emit a warning for non-
UniformCartesian swarms because broader curvilinear swarm behavior remains
mostly untested.
PR Checklist
// This file was made in part with generative AI.