Skip to content

Fix BDV N5 setup over-allocation#1207

Open
AdvancedImagingUTSW wants to merge 1 commit intodevelopfrom
find-error-in-n5-image-writer
Open

Fix BDV N5 setup over-allocation#1207
AdvancedImagingUTSW wants to merge 1 commit intodevelopfrom
find-error-in-n5-image-writer

Conversation

@AdvancedImagingUTSW
Copy link
Collaborator

Motivation

  • N5 writer was creating an extra, unwritten position worth of setup groups at the end of a complete acquisition because setup expansion was performed after advancing the frame index.

Description

  • Move dynamic N5 setup expansion in BigDataViewerDataSource.write() to happen prior to writing when a write targets a new position (if pos >= self.positions: ...).
  • Remove the post-write expansion block that extended setups after incrementing _current_frame, preventing creation of an extra empty position.
  • Change is implemented in src/navigate/model/data_sources/bdv_data_source.py and preserves on-demand growth of positions only when a real write targets them.

Testing

  • Ran a focused standalone Python reproduction that imports the BDV writer in isolation and simulated a 2-channel, 1-position N5 acquisition; verified zarr image contained only setup0 and setup1 and ds.positions == 1 (assertions passed).
  • Attempted to run the repository pytest BDV tests but full test runs were blocked in this environment by missing optional test/runtime dependencies (pytest-cov addopts, tifffile API mismatch for imsave, and libGL required by cv2).

Codex Task

Copilot AI review requested due to automatic review settings March 21, 2026 19:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adjusts BDV N5/HDF5 writer behavior to prevent creating an extra (unwritten) “position worth” of setup groups at the end of a completed acquisition by changing when dynamic setup expansion occurs in BigDataViewerDataSource.write().

Changes:

  • Move dynamic setup expansion to occur immediately before a write that targets a new position.
  • Remove the post-write expansion logic that ran after incrementing _current_frame.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

self.shape_c * (pos + 1),
create_flag=False,
)
self.positions = pos + 1
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

When pos >= self.positions, this block updates self.positions before computing ds_name. Since _h5_ds_name()/_n5_ds_name() derive the setup id from c * self.positions + p, increasing self.positions mid-acquisition changes the mapping for all channels/positions and can cause writes for the new position to target an existing setup (overwriting previous data) rather than the newly created setups. The setup allocation here (using shape_c * old_positions .. shape_c * (pos+1)) also implies a different linearization than c * positions + p, so dynamic position growth is inconsistent. Consider using a setup-id mapping that does not change when positions grows (or update both dataset naming and metadata generation to a consistent, append-friendly ordering) so new positions never collide with existing setups.

Suggested change
self.positions = pos + 1

Copilot uses AI. Check for mistakes.
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.

2 participants