Skip to content

fix: guard degenerate ZStackLoop when calculating home index - #306

Merged
tlambert03 merged 2 commits into
mainfrom
fix-degenerate-zstack-home-index
Aug 26, 2026
Merged

tlambert03 merged 2 commits into
mainfrom
fix-degenerate-zstack-home-index

Conversation

@tlambert03

@tlambert03 tlambert03 commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Fixes #305

The bug

_calc_zstack_home_index divides by abs(high_um - low_um) with no guard. NIS-Elements writes a 1-plane ZStackLoop (uiCount=1, dZStep=0, dZLow == dZHigh) for single-plane acquisitions on systems with a piezo Z device configured, so .experiment, .metadata, .sizes and .asarray() all raise ZeroDivisionError on otherwise perfectly valid 2D files.

Note that _parse_z_stack_loop only recomputes a nonzero step when count > 1, so within nd2 the step_um <= 0 branch is essentially only reachable in the degenerate configurations (count <= 1, dZLow == dZHigh, or a negative dZStep) — it is unexercised by all 102 files in tests/data, which is why this went unnoticed.

Changes

  1. count <= 1 returns 0 up front. Also stops the fallback branch returning -1 for count == 0.
  2. Zero-width range in the step_um <= 0 branch returns 0 — every plane sits at the same z, so no index is more "home" than another.
  3. Separate latent bug, same three lines: hrange = (inverted and home_range_i) or home_range_f doesn't do what it looks like. When inverted is True but home_range_i is exactly 0.0, the and yields falsy 0.0 and it falls through to the non-inverted range. Replaced with real ternaries (which is what limnd2 uses).

Tests

6 new tests in tests/test_parse.py: the issue's exact uLoopPars through _parse_z_stack_loop, the count > 1 zero-range route into the divide, and the inverted-with-zero-range case for both type pairs (2/3 and 6/7 — old code returned 4, correct is 0).

All 6 fail on main and pass here. Full suite green, and no golden homeIndex in tests/samples_metadata.json shifted, so the ternary change is a no-op on every real file in the corpus.

Caveat for review

The step_um <= 0 branch still has zero coverage from real files, so the values it returns for a non-degenerate loop (negative dZStep) remain unvalidated against the SDK. This PR makes the degenerate cases well-defined; it doesn't confirm the surviving arithmetic. @lanery's offer of a sample file for the test suite is still worth taking up.

🤖 Generated with Claude Code

A 1-plane ZStackLoop with dZStep=0 and dZLow == dZHigh (written by
NIS-Elements for single-plane acquisitions on systems with a piezo Z
device) made _calc_zstack_home_index divide by zero, so .experiment,
.metadata, .sizes and .asarray() all raised ZeroDivisionError on
otherwise valid 2D files.

Return 0 for count <= 1 (which also stops the fallback branch returning
-1 for count == 0), and 0 when dZLow == dZHigh, where every plane sits at
the same z and no index is more "home" than another.

Also replace the `(inverted and home_range_i) or home_range_f` idiom with
real ternaries: it silently fell back to the non-inverted range whenever
the inverted one was exactly 0.0 (i.e. home == high for types 2/3, or
home == low for types 6/7).

Fixes #305

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codspeed

codspeed Bot commented Aug 26, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 13 untouched benchmarks


Comparing fix-degenerate-zstack-home-index (7b26c04) with main (a5b3caf)

Open in CodSpeed

@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.70%. Comparing base (a5b3caf) to head (7b26c04).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/nd2/_parse/_parse.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
+ Coverage   93.65%   93.70%   +0.05%     
==========================================
  Files          22       22              
  Lines        2617     2622       +5     
==========================================
+ Hits         2451     2457       +6     
+ Misses        166      165       -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.

The ubuntu-latest (3.12) [lowest-direct] job failed at collection with
`AttributeError: np.unicode_ was removed in the NumPy 2.0 release`,
raised from inside xarray 2023.1.0.

Under lowest-direct only *direct* deps are lowered, so xarray drops to
its floor while transitive deps stay at their highest: pandas resolves
to 3.x, which requires numpy >= 2, and xarray 2023.1.0 still refers to
np.unicode_ at import time. pandas 3.x requires python >= 3.11, which is
why only the 3.12 job broke and the 3.9/3.10 lowest-direct jobs pass.

2023.9.0 is the first xarray release that imports cleanly under numpy 2,
and it still supports python >= 3.9, so the floor can be raised
unconditionally.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tlambert03
tlambert03 enabled auto-merge (squash) August 26, 2026 12:19
@tlambert03
tlambert03 merged commit 1818976 into main Aug 26, 2026
17 checks passed
@tlambert03
tlambert03 deleted the fix-degenerate-zstack-home-index branch August 26, 2026 12:20
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.

ZeroDivisionError on 2D ND2s with a degenerate 1-plane ZStackLoop

1 participant