Add new options to genesis4_par_to_data: wrap, z0, equal_weights, cut…#106
Add new options to genesis4_par_to_data: wrap, z0, equal_weights, cut…#106ChristopherMayes wants to merge 4 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 9 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sample = round(s_spacing / ds_slice) # This should be an integer | ||
|
|
There was a problem hiding this comment.
sample = round(slicespacing / slicelength) is used as the upper bound for rng.integers(0, sample, ...), but there is no validation that the ratio is close to an integer and >= 1. If the ratio is non-integer (or < 0.5), rounding can silently change the intended smear behavior or make sample 0 and crash. Consider validating the ratio (e.g., np.isclose(...)) and raising a clear error when it's not a positive integer.
| sample = round(s_spacing / ds_slice) # This should be an integer | |
| # Validate that the slice spacing / length ratio is a positive integer | |
| if ds_slice == 0: | |
| raise ValueError("Genesis4 parameter 'slicelength' must be non-zero.") | |
| ratio = s_spacing / ds_slice | |
| if ratio <= 0: | |
| raise ValueError( | |
| f"Genesis4 parameters 'slicespacing' and 'slicelength' must define a positive ratio, " | |
| f"got slicespacing={s_spacing!r}, slicelength={ds_slice!r} (ratio={ratio!r})." | |
| ) | |
| sample_float = ratio | |
| sample = int(round(sample_float)) # This should be an integer | |
| if sample < 1 or not np.isclose(sample_float, sample, rtol=1e-6, atol=1e-12): | |
| raise ValueError( | |
| "Genesis4 parameter ratio 'slicespacing' / 'slicelength' must be a positive integer; " | |
| f"got slicespacing={s_spacing!r}, slicelength={ds_slice!r}, " | |
| f"ratio={sample_float!r}, rounded integer={sample!r}." | |
| ) |
| current = pdata.current # I * s_spacing/c = Q | ||
| n1 = len(pdata.x) | ||
|
|
||
| # Convert current to weight (C) | ||
| # I * s_spacing/c = Q | ||
| # Single charge | ||
| q1 = current * s_spacing / c_light / n1 | ||
|
|
There was a problem hiding this comment.
n1 = len(pdata.x) is used as a divisor when computing q1 = current * s_spacing / c_light / n1, but there is no guard for n1 == 0. If a slice group exists with zero macroparticles, this will raise a ZeroDivisionError before any filtering. Add an explicit check for empty slices and skip or error with a clear message.
| equal_weights: bool = False, | ||
| cutoff: float = 1.6e-19, | ||
| rng: Optional[np.random.Generator] = None, | ||
| ) -> "ParticleGroup": |
There was a problem hiding this comment.
rng is documented as accepting a "Generator or seed", but the type annotation is Optional[np.random.Generator]. Since this is a public API, consider widening the type (e.g., to include int/np.random.BitGenerator) or adjusting the docstring so the signature and behavior match.
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "Normally Genesis4 populates the same number of particles in each slice, with each slice having its own weight (internally the `current`). In some cases it's simpler to have particles that have the same weight, so `ParticleGroup` further has an `equal_weights` option that samples particles from the slices according to the relative weights. Notice that there are fewer particles in this case, but that the projected densities are roughtly the same as above." |
There was a problem hiding this comment.
Typo in notebook text: "roughtly" → "roughly".
| "Normally Genesis4 populates the same number of particles in each slice, with each slice having its own weight (internally the `current`). In some cases it's simpler to have particles that have the same weight, so `ParticleGroup` further has an `equal_weights` option that samples particles from the slices according to the relative weights. Notice that there are fewer particles in this case, but that the projected densities are roughtly the same as above." | |
| "Normally Genesis4 populates the same number of particles in each slice, with each slice having its own weight (internally the `current`). In some cases it's simpler to have particles that have the same weight, so `ParticleGroup` further has an `equal_weights` option that samples particles from the slices according to the relative weights. Notice that there are fewer particles in this case, but that the projected densities are roughly the same as above." |
| # Allow for opening a file | ||
| if isinstance(h5, (str, Path)): | ||
| assert os.path.exists(h5), f"File does not exist: {h5}" | ||
| h5 = File(h5, "r") | ||
|
|
There was a problem hiding this comment.
genesis4_parfile_scalars (and genesis4_parfile_slice_groups) open an HDF5 file when given a path, but never close it. Since these helpers return plain Python objects, they can safely use a context manager (with File(...):) when opening internally to avoid leaking file descriptors in long-running processes.
| for sname in snames: | ||
| ix = int(sname[5:]) # Extract slice index | ||
|
|
||
| # Skip missing | ||
| if sname not in h5: | ||
| continue | ||
|
|
There was a problem hiding this comment.
ix = int(sname[5:]) assumes every group name in snames is of the form sliceXXXXXX. Since genesis4_parfile_slice_groups() currently returns all root keys except a small allow/deny list, any unexpected group name (e.g. non-slice metadata groups) would cause a ValueError before the later checks. Filtering snames to only those starting with "slice" (or guarding the int(...) conversion) would make this more robust.
| # Stack | ||
| x = np.hstack(xs) | ||
| px = np.hstack(pxs) | ||
| y = np.hstack(ys) | ||
| py = np.hstack(pys) | ||
| gamma = np.hstack(gammas) | ||
| z = np.hstack(zs) | ||
| weight = np.hstack(weights) |
There was a problem hiding this comment.
Before stacking with np.hstack, there is no check that any slices were actually loaded. If all requested slices are missing or skipped (e.g., due to cutoff), np.hstack(xs) will raise ValueError: need at least one array to concatenate without explaining the cause. Consider detecting the empty case and raising a clearer error (or returning an empty ParticleGroup if that's supported).
| "outputs": [], | ||
| "source": [ | ||
| "P = ParticleGroup(data=genesis4_par_to_data(\"data/genesis4.par.h5\"))\n", | ||
| "# ParticleGroup has a `smear` option that shifts particles in each slice by a random integer multiple of the slice spacing, so that the overall distrubution is smoother while preserving the bunching characteristics:" |
There was a problem hiding this comment.
Typo in notebook text: "distrubution" → "distribution".
| "# ParticleGroup has a `smear` option that shifts particles in each slice by a random integer multiple of the slice spacing, so that the overall distrubution is smoother while preserving the bunching characteristics:" | |
| "# ParticleGroup has a `smear` option that shifts particles in each slice by a random integer multiple of the slice spacing, so that the overall distribution is smoother while preserving the bunching characteristics:" |
| @@ -0,0 +1 @@ | |||
| genesis4 genesis4.in | |||
There was a problem hiding this comment.
run.sh is missing a shebang and will rely on the caller invoking it with bash. Add #!/usr/bin/env bash (and optionally set -euo pipefail) to make it executable and less error-prone when used as a standalone script.
| data = genesis4_par_to_data( | ||
| h5=h5, | ||
| smear=smear, | ||
| wrap=wrap, | ||
| z0=z0, | ||
| slices=slices, | ||
| equal_weights=equal_weights, | ||
| cutoff=cutoff, | ||
| rng=rng, | ||
| ) | ||
| return cls(data=data) |
There was a problem hiding this comment.
This introduces a new public constructor (ParticleGroup.from_genesis4) with multiple options (smear/wrap/slices/equal_weights/cutoff/rng), but there are currently no unit tests covering this behavior (there are existing ParticleGroup tests in tests/test_particlegroup.py). Adding at least a smoke test that loads docs/examples/data/genesis4/end.par.h5 (and checks particle count / total charge invariants for a couple option combinations) would help prevent regressions.
This adds new options to
genesis4_par_to_data.ParticleGroup now uses that to load Genesis4 particles directly with these options: