CI for csaxs_bec / test (push) Successful in 2m14s
Restores the legacy spec behavior of drawing a fresh random offset (uniform in [0, step)) per call and shifting the sampling lattice by it before the FOV crop check -- decorrelates position-tied reconstruction errors that otherwise accumulate into ring artifacts across a ptycho-tomography series. Applied identically to all three generators (get_flomni_fermat_spiral_pos, get_omny_fermat_spiral_pos, get_lamni_fermat_spiral_pos), same scalar offset for x and y, matching the historical spec. New `seed` parameter (default None = fresh randomization) allows deterministic/reproducible calls. The four call sites that predict Fermat point counts ahead of a real scan (flomni.py/lamni.py's _expected_fermat_position_count(), tomo_params.py's _compute_fermat_positions_flomni/_lamni) now pin seed=0 so their own estimates stay reproducible across repeated calls; their docstrings note the real scan's count can differ by a point or two near the threshold due to the randomization. Updated the affected tests to compare against a matching seed=0 instead of an independent random draw. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lx3KffiFyyDMKT8vvPENUW
95 lines
7.2 KiB
Markdown
95 lines
7.2 KiB
Markdown
# Fix needed: restore random sub-step offset in Fermat spiral scans (OMNY, flOMNI, LamNI)
|
|
|
|
**Status:** Implemented on `fixes/flomni_beamtime_2`, pending real/simulated-hardware
|
|
verification. All three generators now draw a fresh `np.random.default_rng(seed).uniform(0,
|
|
step)` offset per call and subtract it from the raw lattice `x`/`y` before the FOV crop check
|
|
(same scalar offset for both axes, matching the legacy spec). Decisions made on the open
|
|
questions below: no scan-metadata logging (a `logger.info` line is emitted instead); the
|
|
opt-out is a `seed: int | None = None` parameter (default `None` = fresh randomization);
|
|
x/y share one scalar offset. The four "preview/estimate" call sites that predict Fermat
|
|
point counts before a real scan runs (`flomni.py`/`lamni.py`'s
|
|
`_expected_fermat_position_count()`, `tomo_params.py`'s `_compute_fermat_positions_flomni`/
|
|
`_compute_fermat_positions_lamni`) now pin `seed=0` so their own repeated-call estimates stay
|
|
reproducible; their docstrings note the real scan's count can differ by a point or two near
|
|
the threshold because of the randomization. Tests updated to match (`seed=0` on the
|
|
comparison calls in `test_fermat_position_warning.py`, `test_tomo_params_widget_math.py`,
|
|
`test_lamni_tomo_params_widget_math.py`); full suite passes (725 passed).
|
|
|
|
**Applies to:** all three setups — **OMNY, flOMNI, and LamNI**. The fix must be applied consistently across all three, not just one.
|
|
|
|
## Background
|
|
|
|
Ptychographic tomography ring artifacts arise from any error that is systematically fixed relative to a common coordinate frame across all projection angles (e.g. a local reconstruction inconsistency always occurring at the same scan-grid position). Randomizing the Fermat scan grid's sub-step phase from projection to projection is a known mitigation: it decorrelates such position-tied errors so they don't accumulate into a ring during backprojection.
|
|
|
|
## What the old spec code did
|
|
|
|
In the legacy spec macros (`spec/scan_handling.mac`, `_rtscan_point_by_point`, fermat branch), a fresh random offset was drawn **on every call** (i.e. every projection) and used to shift the sampling lattice before cropping to the field of view:
|
|
|
|
```c
|
|
_fermat_offset = step*rand(100)/100 // uniform random in [0, step)
|
|
|
|
for (ind0=1; ind0<=_arb_num1; ind0++) {
|
|
_radius = step/sqrt(PI) * sqrt(ind0)
|
|
_angle = 8*PI/(1+sqrt(5))/(1+sqrt(5)) * ind0
|
|
tmp_x = radius*cos(angle)
|
|
tmp_y = radius*sin(angle)
|
|
if (tmp_x in [-(fovx/2)+offset, (fovx/2)+offset] and
|
|
tmp_y in [-(fovy/2)+offset, (fovy/2)+offset]) keep it
|
|
}
|
|
final_x = tmp_x + cenx - _fermat_offset
|
|
final_y = tmp_y + ceny - _fermat_offset
|
|
```
|
|
|
|
Net effect (worked out by algebra): the entire Fermat lattice is shifted by `-offset` before cropping to the FOV window, with `offset` redrawn uniformly from `[0, step)` on every scan. So the exact sub-step "phase" of the sampling lattice relative to the sample/FOV origin varies from projection to projection, rather than being bit-for-bit identical every time.
|
|
|
|
## Current state in BEC (the problem)
|
|
|
|
None of the three current Fermat spiral generators in `csaxs_bec` draw any random number. All three are fully deterministic given `(cenx, ceny, step, fovx, fovy, ...)`:
|
|
|
|
| Setup | File | Function |
|
|
|--------|------|----------|
|
|
| flOMNI | `csaxs_bec/csaxs_bec/scans/flomni_fermat_scan.py` | `get_flomni_fermat_spiral_pos` (staticmethod) |
|
|
| OMNY | `csaxs_bec/csaxs_bec/scans/omny_fermat_scan.py` | `get_omny_fermat_spiral_pos` (instance method) |
|
|
| LamNI | `csaxs_bec/csaxs_bec/scans/lamni_fermat_scan.py` | `get_lamni_fermat_spiral_pos` (staticmethod) |
|
|
|
|
All three share the same core loop shape:
|
|
|
|
```python
|
|
for ii in range(start, n_max):
|
|
radius = step * 0.57 * np.sqrt(ii)
|
|
x = radius * np.sin(ii * phi)
|
|
y = radius * np.cos(ii * phi)
|
|
# ... FOV/circular-FOV crop check ...
|
|
positions.append([x + cenx, y + ceny, z_pos])
|
|
```
|
|
|
|
Since `cenx`/`ceny` come from the per-angle alignment/offset computation (not randomized), every projection at a given nominal center currently samples the **exact same** lattice phase relative to the sample. This is a plausible, currently-unmitigated contributor to ring artifacts in reconstructions from all three instruments.
|
|
|
|
Note: there is an existing `random.uniform` call in `csaxs_bec/csaxs_bec/bec_ipython_client/plugins/flomni/flomni.py` (`single_point_random_shift_max`, used in `tomo_acquire_at_angle`). That is a **separate, unrelated** mechanism for single-point burst acquisitions — it does not touch the Fermat spiral generators and does not address this issue.
|
|
|
|
## What needs to happen
|
|
|
|
1. Add a per-call random sub-step offset to all three spiral generators (`get_flomni_fermat_spiral_pos`, `get_omny_fermat_spiral_pos`, `get_lamni_fermat_spiral_pos`), reproducing the intent of the old spec `_fermat_offset` (uniform draw in `[0, step)`, applied as a lattice-phase shift before FOV cropping).
|
|
2. Apply the fix **identically in spirit** across OMNY, flOMNI, and LamNI — the three functions currently share near-identical structure and should stay in sync on this behavior. Watch for the small existing differences between them (flOMNI/LamNI take `cenx`/`ceny` as static args; OMNY reads `self.cenx`/`self.ceny`; LamNI has additional stitching/shift/circular-FOV parameters that the offset must compose correctly with).
|
|
3. Decide and document (open questions below) rather than guessing silently.
|
|
|
|
## Open questions for the implementing session to resolve (and record the decision)
|
|
|
|
- **Reproducibility/logging:** should the drawn offset be logged into scan metadata (e.g. via `update_scan_info`) so a given projection's exact grid phase can be recovered/audited later, the way `_arb_num1`/positions already are?
|
|
- **Opt-out for testing/repeatability:** should there be a parameter (e.g. `randomize_offset: bool = True`, or an explicit `seed`) so existing/new unit tests and commissioning scans can request deterministic output?
|
|
- **X/Y coupling:** the original spec used the *same* scalar `_fermat_offset` for both the x- and y-shift. Decide whether to preserve that (simpler, matches historical behavior) or draw independent offsets per axis (stronger decorrelation, but diverges from the reference implementation) — pick one and note the reasoning.
|
|
- **Interaction with existing FOV/circular-FOV cropping logic**, especially in LamNI's `_lamni_check_pos_in_fov_range_and_circ_fov` and any stitching offsets — the offset must shift the lattice consistently with those checks, not just the final translation.
|
|
|
|
## Tests likely needing updates
|
|
|
|
- `csaxs_bec/csaxs_bec/tests/tests_scans/test_flomni_fermat_scan.py`
|
|
- `csaxs_bec/csaxs_bec/tests/tests_scans/test_omny_fermat_scan.py`
|
|
- `csaxs_bec/csaxs_bec/tests/tests_scans/test_lamni_fermat_scan.py`
|
|
|
|
Any test currently asserting exact/deterministic position arrays will need either a fixed seed, the opt-out parameter above, or an update to assert distributional/structural properties instead of exact values.
|
|
|
|
## Non-goals
|
|
|
|
- Do not change the underlying spiral density/index-to-radius/angle formula (`radius = step * 0.57 * sqrt(ii)`, golden-angle `phi`) — only the lattice-phase (offset) behavior is in scope.
|
|
- Do not touch `single_point_random_shift_max` / `tomo_acquire_at_angle` in `flomni.py` — unrelated mechanism, out of scope here.
|