Skip to content

feat: port normalization to NeuNorm 2.0 (route a, #412)#420

Open
KedoKudo wants to merge 2 commits into
nextfrom
port/neunorm-2.0-core
Open

feat: port normalization to NeuNorm 2.0 (route a, #412)#420
KedoKudo wants to merge 2 commits into
nextfrom
port/neunorm-2.0-core

Conversation

@KedoKudo

@KedoKudo KedoKudo commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

⚠️ DO NOT MERGE — awaiting CIS hands-on testing

This is a load-bearing PR under the flagship rule: it swaps the normalization engine that produces every downstream number (binning → fitting → strain). CI green and review are not sufficient. Jean (CIS) must complete the side-by-side protocol below on real data first.

What

The route-(a) NeuNorm 2.0 port decided 2026-06-12 (#412):

  • core/processing/normalization.py — the transmission division now runs through neunorm.processing.normalizer.normalize_transmission on scipp DataArrays. The 1.x behaviors that 2.0 dropped are bridged locally in a new single math path, normalize_stacks():
    • in-memory stacks (2.0 loaders are path-based); float32 working dtype
    • pooled multi-ROI background correction in the 1.x ratio-of-means form, with the inclusive ROI extents (a width-w ROI covers w+1 pixels)
    • per-frame 1:1 OB pairing when stack counts match; nanmedian(OB) fallback when they don't
    • zero-OB pixels (NaN/inf) zeroed; the sample-only path does not zero (1.x asymmetry, now pinned by a test)
    • per-frame normalized_image_NNNN.tif float32 export — the on-disk layout step3's folder re-import depends on
    • no variances attached: iBeatles stacks may be pre-smoothed or contain negative pixels, so Poisson statistics would be wrong
  • step2 GUI — the parallel NeuNorm implementation is gone; the GUI now delegates to normalize_stacks(), keeping its dialogs, status messages, session bookkeeping, and the step3 auto re-import.
  • Writer-only sites (step6 image exports, tof_bin ×2, tof_combine) — NeuNorm replaced by core/io/tiff.write_float32_tiff (PIL, the same writer 1.x used). On-disk names are preserved exactly, including 1.x's quirk of rewriting step6's .tiff base names to .tif.
  • About box — reports neunorm: 2.0.0; the duplicated version blocks are deduped.
  • Packagingneunorm>=2.0.0,<3 + scipp in pyproject/pixi/environment.yml; the conda recipe's 1.x pip-vendoring workaround is replaced by a real neunorm run requirement (2.0 publishes a clean conda package to neutronimaging — the recipe's own TODO comment). Lockfile resolved: neunorm 2.0.0 (PyPI wheel), scipp 26.3.1 (conda-forge).

Numeric gate

  • The 9 value-pinning contract tests from test: pin normalize_data numeric semantics ahead of the NeuNorm 2.0 port #415 pass unmodified — they were written against pinned NeuNorm 1.6.12 with formula-based expectations and are the port's acceptance gate.
  • Full suite: 223 passed, pre-commit clean.
  • Expected agreement with 1.x on real data: float32 precision (rtol ≤ 1e-5), not bit-identity — 1.x accumulated sums in float32; the port computes in float64 and rounds once at the end. Differences should be last-ulp only.

Behavior preserved vs changed

Preserved on purpose so this PR changes one variable at a time (both are audit findings; fixes are in the coordinated science-fixes PR, not here):

  1. The GUI moving average never smooths the OB (the 1.x-era OB branch was dead code).
  2. In the "normalization then moving average" order the moving average still runs (status messages, failure fails the run) but cannot affect the normalized output — exactly as today.

Changed (UX/logging only, no numbers):

  • no-OB + no-background-ROI now fails through the status bar instead of letting ValueError escape the Qt slot (PyQt5 aborts on that);
  • a moving-average failure in the "average first" order now fails gracefully instead of crashing on None;
  • step6's integrated-image log line says "integrated image" (was a copy-paste "strain mapping") and logs the real .tif path;
  • import inflect removed from tof_bin/export_images.py — the package was never declared in any manifest nor present in any lockfile, so the module-level import was a latent crash; it only pluralized one log word.

Testing protocol for CIS (Jean)

Setup: current release (1.x engine) vs this branch, same machine, same data. Suggested comparison: np.testing.assert_allclose(old, new, rtol=1e-5) over the exported TIFFs, plus end-to-end strain/d-spacing outputs. Run with moving average OFF for the numeric comparisons (the smoothing axis itself is a known bug being fixed separately; with MA on, compare only "runs and exports without errors").

# Scenario What to compare
1 Sample + OB, equal frame counts, no background ROI exported normalized_*.tif values; step3 auto-import looks right
2 Sample + OB, equal counts, 2+ background ROIs exported values (exercises pooled multi-ROI + inclusive extents)
3 Sample + OB with different frame counts exported values (exercises the nanmedian(OB) fallback)
4 Sample only + background ROI(s) exported values
5 Sample only, no ROI error message in the status bar; app does not crash
6 Dataset with zero-count OB pixels (or synthetic) those pixels are exactly 0 in the output
7 CLI pipeline (ibeatles --no-gui with a config) end-to-end normalized TIFFs + fitting/strain outputs vs current release
8 step6 exports (d-spacing, strain map, integrated) identical file names; pixel values bit-identical (writer swap only)
9 tof_bin and tof_combine exports identical names/values; metadata.json + Spectra txt unchanged
10 About box shows neunorm: 2.0.0, single entry per library

Acceptance: scenarios 1–7 agree within float32 precision; 8–9 bit-identical; 5 fails loud-but-graceful; no GUI flow regressions.

Deferred (next PR, also CIS-tested)

Science fixes that change published numbers, kept out of the engine swap deliberately: moving-average axis (smooths wavelength/TOF instead of spatial), the "Normalization, Moving Average" order no-op, step6 strain-error formula, spectra empty-bin alignment, shipped config.json debugging: true.

Also queued for that batch (surfaced by Copilot review, pre-existing in the current release): tof_bin's metadata.json and logs record image_NNNN.tif names while the files land as .tiff — a 1.x export(file_type="tiff") extension-rewrite quirk this PR preserves byte-for-byte on purpose.

Closes #412 (with the science-fixes PR completing the epic).

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 53.28947% with 71 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.01%. Comparing base (b0e83ee) to head (bd739cd).

Files with missing lines Patch % Lines
src/ibeatles/step2/normalization.py 0.00% 45 Missing ⚠️
src/ibeatles/step6/export.py 0.00% 11 Missing ⚠️
src/ibeatles/tools/tof_bin/export_images.py 0.00% 5 Missing ⚠️
src/ibeatles/about/about_launcher.py 0.00% 3 Missing ⚠️
.../ibeatles/tools/tof_bin/tof_bin_export_launcher.py 0.00% 3 Missing ⚠️
src/ibeatles/core/processing/normalization.py 97.29% 1 Missing and 1 partial ⚠️
...ibeatles/tools/tof_combine/export/export_images.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next     #420      +/-   ##
==========================================
+ Coverage   10.68%   11.01%   +0.33%     
==========================================
  Files         195      196       +1     
  Lines       17827    17832       +5     
  Branches     1829     1835       +6     
==========================================
+ Hits         1905     1965      +60     
+ Misses      15875    15819      -56     
- Partials       47       48       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Core engine swap, gated by the value-pinning contract tests of #415
(all 9 pass unmodified, plus 223-test suite green):

- core/processing/normalization.py: the transmission division runs
  through neunorm.processing.normalizer.normalize_transmission on
  scipp DataArrays; the 1.x behaviors 2.0 dropped are bridged locally
  (in-memory stacks, float32 working dtype, pooled multi-ROI inclusive
  air correction, 1:1 vs nanmedian(OB) pairing, zero-OB zeroing,
  per-frame normalized_image_NNNN.tif export)
- normalize_stacks() is the single math path; step2's parallel GUI
  implementation now delegates to it, keeping its dialogs, status
  messages and step3 re-import. Two audit-documented no-ops (OB never
  smoothed; "normalization then moving average" not reaching outputs)
  are preserved on purpose so this PR changes one variable at a time
- the four TIFF-writer-only sites (step6 export, tof_bin x2,
  tof_combine) drop NeuNorm for core/io/tiff.write_float32_tiff,
  preserving exact on-disk names including the 1.x extension rewrite
- about box reports neunorm (duplicated version blocks deduped)
- deps: neunorm >=2.0.0,<3 + scipp (pyproject/pixi/environment.yml);
  conda recipe replaces the 1.x pip-vendoring with a real neunorm
  run requirement; lockfile resolved (neunorm 2.0.0, scipp 26.3.1)
- drop module-level `import inflect` in tof_bin/export_images.py: the
  package was never declared nor locked, so the import was a latent
  crash; it only pluralized a log word

DO NOT MERGE until CIS (Jean) completes the side-by-side testing
protocol in the PR description (flagship rule).

Assisted-With: Claude Fable 5 (1M context) <noreply@anthropic.com>
@KedoKudo KedoKudo force-pushed the port/neunorm-2.0-core branch from 0f58158 to 744c5b2 Compare June 12, 2026 18:48
@KedoKudo KedoKudo requested a review from Copilot June 12, 2026 18:57
@KedoKudo KedoKudo self-assigned this Jun 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Ports iBeatles’ normalization pipeline to NeuNorm 2.0 by routing the core transmission normalization through neunorm.processing.normalizer.normalize_transmission (via scipp DataArrays), while preserving key NeuNorm 1.x numeric semantics and on-disk TIFF export layout relied on by downstream steps/tools.

Changes:

  • Introduces a single shared normalization math path (normalize_stacks) plus export_normalized_stack and a NeuNorm-1.x-compatible float32 TIFF writer (write_float32_tiff).
  • Refactors the step2 GUI normalization flow to delegate computation/export to the shared core normalization helpers.
  • Replaces NeuNorm-based “writer-only” export sites (step6, tof_bin, tof_combine) with the new float32 TIFF writer and updates dependencies for NeuNorm 2.0 + scipp.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/ibeatles/core/processing/normalization.py Implements NeuNorm 2.0-backed normalization (normalize_stacks) and preserves 1.x semantics + per-frame export layout.
src/ibeatles/core/io/tiff.py Adds a dedicated float32 single-page TIFF writer to replace NeuNorm export usage.
src/ibeatles/step2/normalization.py Updates GUI normalization to use normalize_stacks() + export_normalized_stack() while retaining GUI plumbing.
src/ibeatles/step6/export.py Replaces NeuNorm export with write_float32_tiff, preserving .tif naming behavior.
src/ibeatles/tools/tof_bin/tof_bin_export_launcher.py Switches bin export TIFF writing from NeuNorm to write_float32_tiff.
src/ibeatles/tools/tof_bin/export_images.py Switches export-images TIFF writing from NeuNorm to write_float32_tiff and removes inflect usage.
src/ibeatles/tools/tof_combine/export/export_images.py Switches combine export TIFF writing from NeuNorm to write_float32_tiff.
src/ibeatles/about/about_launcher.py Updates “About” dependency reporting from NeuNorm to neunorm and removes duplicate blocks.
tests/unit/ibeatles/core/processing/test_normalize_stacks.py Adds unit tests for input handling, ROI bounds, sample-only edge behavior, and export naming.
tests/unit/ibeatles/core/io/test_tiff.py Adds unit tests for float32 TIFF writer round-trip and input validation.
pyproject.toml Updates dependencies to neunorm>=2,<3 and adds scipp.
environment.yml Updates environment dependencies for NeuNorm 2.0 + scipp.
conda.recipe/meta.yaml Removes pip-vendored NeuNorm install and declares neunorm + scipp runtime requirements.
pixi.lock Locks new dependency resolution including scipp and neunorm 2.0.0.

Comment thread src/ibeatles/tools/tof_bin/tof_bin_export_launcher.py
Comment thread src/ibeatles/tools/tof_bin/export_images.py
Comment thread src/ibeatles/step2/normalization.py
Copilot review on #420: the caught ValueError (e.g. no OB + no
background ROI) was only logged, leaving the user with the generic
"Normalization Failed" message. running_normalization now records the
failure reason and run_and_export shows it in the status bar; the ROI
table retrieval error gets a specific message too.

Assisted-With: Claude Fable 5 (1M context) <noreply@anthropic.com>
@KedoKudo KedoKudo marked this pull request as ready for review June 12, 2026 19:36
@KedoKudo KedoKudo requested a review from JeanBilheux June 12, 2026 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pin NeuNorm to pre-2.0 (stopgap) and port iBeatles to NeuNorm 2.0 API

2 participants