Add t_offset Argument to ParticleGroup.write - #152
Conversation
ken-lauer
left a comment
There was a problem hiding this comment.
If you and @ChristopherMayes say the round-trippability doesn't matter, then I'm fine with this proceeding as-is.
| name : str, optional | ||
| Subgroup to create for the bunch. If None, writes directly into `h5`. | ||
| t_offset : float or numpy.ndarray, optional | ||
| Time offset, scalar or per-particle, written as the 'timeOffset' record. |
There was a problem hiding this comment.
Per-particle offset seems a strange thing to me (as clearly a non-physicist), but apparently it's a reasonable thing?
The standard does indicate this, after all:
The reference time may depend upon the longitudinal position of a particle and so may be different for different particles.
There was a problem hiding this comment.
Yes, I allowed offsets because it's in the standard. I also don't have an immediate use, but if it's allowed in the standard, we might as well expose it.
There was a problem hiding this comment.
The time offset itself seems to be special enough to deserve its own spot in ParticleGroup. Well, at least from my usual standpoint of "if your data isn't round-trippable (de/serializable) to its original representation it's a problem".
Trying to think this through a bit:
# assume timeOffset is set in particles.h5
P = ParticleGroup("particles.h5") # defaults to include_offset=True
P.t # includes timeOffset from the original file
# P.time_offset ❌ not retrievable or otherwise inspectable
P.write("out.h5", t_offset=0.0) # not round-trippable / lossy: timeOffset goes awayThe test suite does:
ParticleGroup(
data=load_bunch_data(hf[particle_paths(hf)[0]], include_offset=False)
)And then allows it to be handled on the side. This include_offset flag opts out of all offsets wholesale though, so you can't just opt out of time offset handling. A bit awkward, I think.
There was a problem hiding this comment.
Yeah, I think it's up to @ChristopherMayes if he wants to start including the offsets. I agree with you on being able to round-trip in principle. This was the smallest atomic edit I could make to avoid the numerical artifacts without doing a big architecture change of ParticleGroup.
If we wanted to think about a bigger change, one path forward could be to promote x, y, z, t... to properties and have the actual fields be raw_x, offset_x with .x returning raw_x + offset_x. This would keep the current behavior while supporting the offset fields (which are in the OpenPMD standard). For the sake of small PRs and keeping work flowing, it might be best to start here and think about the bigger ParticleGroup change in a separate issue?
edit: Just read below comment, I will merge and add the offset notes as a suggestion in an issue
ken-lauer
left a comment
There was a problem hiding this comment.
Well, I don't like it but @ChristopherMayes said it's the design intent.
In short, LGTM.
This PR adds the new argument
t_offsettoParticleGroup.write. This gets written as the OpenPMDtimeOffsetattribute to avoid numerical roundoff error when writingParticleGroupobjects with reference time included for some codes (ie Bmad which reads them separately and stores internally as a quad precision floating point number.Full changes:
t_offsetinParticleGroup.writesupporting scalar and array types. Does not writetimeOffsetwhen not specified.write_pmd_bunchdocstring to numpyt_offsettodocs/examples/write_examples.ipynbScreenshot