-
-
Notifications
You must be signed in to change notification settings - Fork 19
Add t_offset Argument to ParticleGroup.write
#152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
2f9bd92
3d8f9ef
7048233
b3f3566
7bf4511
3059f70
fadebd0
fc2b82c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,19 +38,27 @@ def pmd_field_init(h5, externalFieldPath="/ExternalFieldPath/%T/"): | |
| h5.attrs[k] = fstr(v) | ||
|
|
||
|
|
||
| def write_pmd_bunch(h5, data, name=None): | ||
| def write_pmd_bunch(h5, data, name=None, t_offset=0.0): | ||
| """ | ||
| Data is a dict with: | ||
| np.array: 'x', 'px', 'y', 'py', 'z', 'pz', 't', 'status', 'weight' | ||
| str: 'species' | ||
| int: n_particle | ||
|
|
||
| Optional data: | ||
| np.array: 'id' | ||
|
|
||
| See inverse routine: | ||
| .particles.load_bunch_data | ||
|
|
||
| Write bunch data in openPMD-beamphysics format. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| h5 : h5py.File or h5py.Group | ||
| Handle to write into. | ||
| data : dict or ParticleGroup | ||
| Requires keys 'x', 'px', 'y', 'py', 'z', 'pz', 't', 'status', 'weight' | ||
| (arrays), 'species' (str), 'n_particle' (int), 'charge' (float). | ||
| Optional key: 'id' (array). | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per-particle offset seems a strange thing to me (as clearly a non-physicist), but apparently it's a reasonable thing?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| Omitted when zero. Readers add this to the 'time' record. Default is 0.0. | ||
|
|
||
| See Also | ||
| -------- | ||
| beamphysics.particles.load_bunch_data : inverse routine | ||
| """ | ||
| if name: | ||
| g = h5.create_group(name) | ||
|
|
@@ -82,6 +90,18 @@ def write_pmd_bunch(h5, data, name=None): | |
| if "id" in data: | ||
| g["id"] = data["id"] | ||
|
|
||
| # Optional time offset, with the same shape as the particle arrays. | ||
| t_offset = np.asarray(t_offset, dtype=float) | ||
| if np.any(t_offset): | ||
| n_particle = data["n_particle"] | ||
| if t_offset.ndim == 0: | ||
| t_offset = np.broadcast_to(t_offset, (n_particle,)) | ||
| elif t_offset.shape != (n_particle,): | ||
| raise ValueError( | ||
| f"t_offset shape {t_offset.shape} does not match n_particle {n_particle}" | ||
| ) | ||
| write_component_data(g, "timeOffset", t_offset, unit=pg_units("t")) | ||
|
|
||
|
|
||
| def write_pmd_field(h5, data, name=None): | ||
| """ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
The test suite does:
And then allows it to be handled on the side. This
include_offsetflag opts out of all offsets wholesale though, so you can't just opt out of time offset handling. A bit awkward, I think.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 beraw_x,offset_xwith.xreturningraw_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 biggerParticleGroupchange in a separate issue?@ChristopherMayes?
edit: Just read below comment, I will merge and add the offset notes as a suggestion in an issue