Add acquisition metadata extraction for XY and Z scale

[hongquanli]

Summary

• Extract physical pixel sizes from OME-TIFF metadata (PhysicalSizeX/Y/Z from OME-XML)
• Read pixel size and dz from acquisition_parameters.json for single-TIFF datasets
• Read pixel size from TIFF metadata tags (ImageDescription JSON or XResolution/YResolution)
• Apply voxel scale transform for correct 3D aspect ratio (no data interpolation)
• Log scale information when data is loaded

Details

Metadata Sources

OME-TIFF:

• Parses PhysicalSizeX, PhysicalSizeY, PhysicalSizeZ from OME-XML metadata
• Supports multiple OME namespace versions (2016-06, 2015-01, 2013-06)
• Handles unit conversion (nm, mm, m → micrometers)

Single-TIFF (in priority order):

1. acquisition_parameters.json in the dataset directory
2. TIFF ImageDescription tag (JSON with keys like pixel_size_um)
3. TIFF XResolution/YResolution tags with inch or cm units

3D Aspect Ratio Correction

When physical pixel sizes are known, the 3D rendering displays correct proportions:

1. Compute Z scale factor: z_scale = dz / pixel_size_xy
2. Apply STTransform(scale=(1, 1, z_scale)) to the vispy Volume visual
3. Volume appears with correct physical proportions

Example: If pixel_size_xy = 0.325 µm and dz = 1.5 µm:

• z_scale = 1.5 / 0.325 = 4.615
• Z dimension appears 4.615× larger in 3D view (matching physical reality)

Benefits over data resampling:

• No interpolation artifacts
• No additional memory usage
• Instant application (no computation)

Scale Metadata Storage

Physical pixel sizes stored in xarray attrs:

• pixel_size_um - XY pixel size (µm)
• dz_um - Z step size (µm)
• pixel_size_x_um, pixel_size_y_um, pixel_size_z_um - individual axes

Test plan

• 17 unit tests for metadata extraction (OME, JSON, TIFF tags)
• 1 test verifying pixel size attrs preserved
• All 31 tests pass
• Visual test: load data with known anisotropic voxels, verify 3D proportions

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR adds acquisition metadata extraction capabilities to enable correct 3D aspect ratio rendering in the ndviewer_light application. The implementation extracts physical pixel sizes from various sources and applies voxel scale transformations for proper 3D visualization of anisotropic data.

Key changes:

• Metadata extraction functions for OME-TIFF, acquisition_parameters.json, and TIFF tags
• Vispy VolumeVisual monkey-patching to support anisotropic voxel rendering via vertex scaling
• Storage of physical pixel sizes in xarray attributes for downstream use

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

File	Description
tests/test_metadata_extraction.py	Comprehensive test suite (17 tests) for metadata extraction from OME-XML, JSON parameters, and TIFF tags
tests/test_downsampling_wrapper.py	Added test to verify pixel size attributes are preserved through the downsampling wrapper
tests/test_3d_visualization.py	Manual visual test script for verifying 3D aspect ratio with synthetic anisotropic volumes
ndviewer_light.py	Core implementation: metadata extraction functions, vispy patching for anisotropic rendering, and integration into data loading pipeline

---

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

[Copilot]

The patched _create_vertex_data method replaces the original implementation entirely but doesn't call the original method. If the original _create_vertex_data performs additional necessary setup beyond setting vertices and indices, this patch could break functionality. Consider calling the original method first and then modifying only the vertex positions, or at minimum document why completely replacing the method is safe.

[hongquanli]

[Claude Code] Fixed in commit 7e69248 - now patches VolumeVisual.init to capture scale at construction, stores as instance attribute instead of relying on global.

[Copilot]

The function reads pixel size from JSON metadata without validating that the value is positive or within reasonable bounds before applying the sanity check. If the JSON contains a negative or zero pixel_size_um, float(metadata[key]) will succeed but the sanity check will reject it. However, there's no validation to prevent negative values being returned if they somehow pass the sanity check. Consider adding explicit validation: val > 0 before the range check.

Suggested change

	# Try ImageDescription tag FIRST for JSON metadata
	val = float(metadata[key])
	# Explicitly require a strictly positive pixel size
	if val <= 0:
	continue

[hongquanli]

[Claude Code] Fixed in commit 7e69248 - added explicit val <= 0 check before sanity check.

[Copilot]

The sanity check range (0.01 < val < 100) for pixel sizes is reasonable for most microscopy, but the upper bound of 100 micrometers might be too restrictive for some low-magnification imaging applications. For example, a 2x objective with a large-pixel camera could legitimately have pixel sizes around 50-150 micrometers. Consider documenting this limitation or making the range configurable.

[hongquanli]

[Claude Code] Fixed in commit 7e69248 - added documentation comment explaining the 0.01-100 µm range covers most use cases including low-mag imaging.

[Copilot]

The global variable _current_voxel_scale is accessed and modified without thread safety mechanisms. If the viewer is used in a multi-threaded context (e.g., loading data from multiple sources or multiple viewer instances), this could lead to race conditions where the wrong scale is applied to a volume.

[hongquanli]

[Claude Code] Fixed in commit 7e69248 - voxel scale is now stored as instance attribute on VolumeVisual during init, eliminating race conditions.

[Copilot]

The docstring states that NDV assumes isotropic voxels, but the PR description and implementation in ndviewer_light.py shows that the code now applies voxel scale transformations for anisotropic voxels via the vispy VolumeVisual patch. This comment is misleading or outdated.

Suggested change

	reference but don't affect downsampling (NDV assumes isotropic voxels).
	reference and do not affect the numerical downsampling, which operates
	in index space; voxel scaling for display is handled separately.

[hongquanli]

[Claude Code] Fixed in commit 7e69248 - updated docstring to clarify that pixel sizes don't affect numerical downsampling (index space), voxel scaling for display handled separately via vispy patch.

[Copilot]

The patched _create_vertex_data method accesses the global _current_voxel_scale but does not handle race conditions. If multiple volumes are being created simultaneously, they might use the wrong scale factor. Consider storing the scale as an attribute on the VolumeVisual instance instead of using a global variable.

Suggested change

	def _patched_create_vertex_data(self):
	"""Create vertices with Z scaling for anisotropic voxels."""
	global _current_voxel_scale
	shape = self._vol_shape
	# Get corner coordinates with Z scaling
	x0, x1 = -0.5, shape[2] - 0.5
	y0, y1 = -0.5, shape[1] - 0.5
	# Apply Z scale if set
	if _current_voxel_scale is not None:
	sz = _current_voxel_scale[2]
	_orig_init = VolumeVisual.__init__
	def _patched_init(self, *args, **kwargs):
	"""Initialize VolumeVisual and capture the current voxel scale."""
	_orig_init(self, *args, **kwargs)
	# Capture the voxel scale active at construction time
	global _current_voxel_scale
	if _current_voxel_scale is not None:
	self._voxel_scale = _current_voxel_scale
	else:
	# Explicitly store None so getattr checks are consistent
	self._voxel_scale = None
	VolumeVisual.__init__ = _patched_init
	def _patched_create_vertex_data(self):
	"""Create vertices with Z scaling for anisotropic voxels."""
	shape = self._vol_shape
	# Get corner coordinates with Z scaling
	x0, x1 = -0.5, shape[2] - 0.5
	y0, y1 = -0.5, shape[1] - 0.5
	# Apply Z scale if set on this instance
	scale = getattr(self, "_voxel_scale", None)
	if scale is not None:
	sz = scale[2]

[hongquanli]

[Claude Code] Fixed in commit 7e69248 - implemented suggested approach: patch VolumeVisual.init to capture scale at construction and store as self._voxel_scale instance attribute.

[Copilot]

Test is always true, because of this condition.

Suggested change

	# Only use resolution tags if we have a proper unit (inch=2 or cm=3)
	unit_value = res_unit.value if res_unit else 1
	if unit_value not in (2, 3):
	return None # No unit or unknown unit - can't reliably convert
	if x_res is not None:
	# XResolution is stored as a fraction (numerator, denominator)
	x_res_value = x_res.value
	if isinstance(x_res_value, tuple) and len(x_res_value) == 2:
	pixels_per_unit = x_res_value[0] / x_res_value[1]
	if unit_value == 2: # inch
	# pixels/inch -> um/pixel: 25400 um/inch / pixels_per_inch
	pixel_size_um = 25400.0 / pixels_per_unit
	else: # centimeter (unit_value == 3)
	# pixels/cm -> um/pixel: 10000 um/cm / pixels_per_cm
	pixel_size_um = 10000.0 / pixels_per_unit
	if 0.01 < pixel_size_um < 100:
	# Sanity check: typical microscopy pixel sizes are 0.1-10 um
	return pixel_size_um

[hongquanli]

[Claude Code] Fixed in commit 7e69248 - removed redundant 'if pixels_per_unit > 0' check since the preceding 'if pixels_per_unit <= 1: return None' already ensures positive value.

[Copilot]

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

	if desc is not None:
	desc_str = desc.value
	except (json.JSONDecodeError, ValueError, TypeError) as exc:
	# Failed to parse pixel size from JSON; fall back to resolution tags below.
	logging.debug(
	"Could not parse pixel size from TIFF ImageDescription JSON: %s",
	exc,
	)

[hongquanli]

[Claude Code] Fixed in commit 7e69248 - added explanatory comment for the except clause.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

---

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

[Copilot]

The VolumeVisual.init is assigned twice, first on line 92 and again on line 137. The second assignment on line 137 is redundant since the patched version was already assigned on line 92. This could lead to confusion when debugging or maintaining the code.

Suggested change

VolumeVisual.__init__ = _patched_init

[hongquanli]

[Claude Code] Fixed in commit 33cbf3c - removed duplicate init assignment on line 137.

[Copilot]

The comment states the order is "ImageDescription tag (JSON metadata from some microscopy software)" then "XResolution/YResolution tags", but the actual docstring in the function says the opposite order (1. XResolution/YResolution, 2. ImageDescription). The comment should match the docstring order for consistency, or the docstring should be updated to reflect the actual priority order implemented in the code (ImageDescription is checked FIRST, as shown by the code and the inline comment on line 567).

Suggested change

	1. XResolution/YResolution tags with ResolutionUnit (inch or cm only)
	2. ImageDescription tag (JSON metadata from some microscopy software)
	1. ImageDescription tag (JSON metadata from some microscopy software)
	2. XResolution/YResolution tags with ResolutionUnit (inch or cm only)

[hongquanli]

[Claude Code] Fixed in commit 33cbf3c - updated docstring to reflect actual code priority (ImageDescription first, then XResolution/YResolution).

[Copilot]

The double negation 'not pytest.importorskip' combined with 'skipif' creates a confusing double negative. The condition should be simplified to use a positive check. Additionally, the reason string is duplicated - it appears both in the importorskip call and as the skipif reason parameter, which is redundant.

[hongquanli]

[Claude Code] Fixed in commit 33cbf3c - replaced confusing skipif(not importorskip(...)) with direct pytest.importorskip() call at test start.

[Copilot]

The double negation 'not pytest.importorskip' combined with 'skipif' creates a confusing double negative that should be simplified to use a positive check. Additionally, the reason string is duplicated between the importorskip call and the skipif reason parameter.

[hongquanli]

[Claude Code] Fixed in commit 33cbf3c - replaced confusing skipif(not importorskip(...)) with direct pytest.importorskip() call at test start.

[Copilot]

The double negation 'not pytest.importorskip' combined with 'skipif' creates a confusing double negative that should be simplified to use a positive check. Additionally, the reason string is duplicated between the importorskip call and the skipif reason parameter.

[hongquanli]

[Claude Code] Fixed in commit 33cbf3c - replaced confusing skipif(not importorskip(...)) with direct pytest.importorskip() call at test start.

[Copilot]

The variable 'tmpdir' should be named 'tmp_dir' to follow Python naming conventions (snake_case with underscores). The current naming without underscore is inconsistent with the 'subdir' variable below and Python PEP 8 style guidelines.

[hongquanli]

[Claude Code] Fixed in commit 33cbf3c - renamed tmpdir to tmp_dir for PEP 8 snake_case compliance.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

ndviewer_light.py:1221

• When pixel_size_x and pixel_size_y differ (non-isotropic XY pixels), averaging them at line 1221 can be misleading. While many microscopes have square pixels, some systems may have slightly non-square pixels. Consider logging a warning when pixel_size_x and pixel_size_y differ significantly (e.g., by more than 1%), or storing both values separately and using the X value for the averaged pixel_size_um (since it's often the primary scan direction).

            # Ensure standard dims exist with singleton axes if missing
            for ax in ["time", "fov", "z", "channel", "y", "x"]:
                if ax not in xarr.dims:

---

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

[Copilot]

The docstring mentions "inch or cm only" for ResolutionUnit but doesn't explain what happens with other unit values (unit_value=1 is "no unit"). Consider documenting that unit_value=1 (no absolute unit) is explicitly rejected at line 602-603 to avoid ambiguous conversions, and that only inch (2) and cm (3) units are supported for reliable physical size extraction.

[hongquanli]

[Claude Code] Fixed in commit 998510d - updated docstring to explain that unit_value=1 (no absolute unit) is explicitly rejected because it cannot be reliably converted to physical units.

[Copilot]

The temporary directory created at line 38 is never explicitly cleaned up. While Python's garbage collector may eventually remove it, the temporary directory and all test TIFF files will persist on disk until manual cleanup or system reboot. Consider using a context manager or explicitly removing the directory in a finally block, or document that users should manually clean up /tmp/ndv_3d_test_* directories after testing.

[hongquanli]

[Claude Code] Fixed in commit 998510d - added note in docstring explaining that temp directories are intentionally NOT cleaned up to allow post-test inspection, with manual cleanup command.

[Copilot]

The dz parameter extracted from acquisition_parameters.json is not validated to be positive. Negative or zero Z-step sizes are physically meaningless and could cause division by zero or other issues downstream (e.g., when computing z_scale at line 1388). Add validation to ensure dz is strictly positive before returning it, similar to the sanity checks applied to pixel_size in read_tiff_pixel_size.

Suggested change

	dz = float(params[key])
	break
	try:
	candidate_dz = float(params[key])
	except (TypeError, ValueError):
	# Skip non-numeric dz values
	continue
	# dz must be strictly positive to be physically meaningful
	if candidate_dz > 0.0:
	dz = candidate_dz
	break

[hongquanli]

[Claude Code] Fixed in commit 998510d - added validation requiring dz to be strictly positive before accepting it.

[Copilot]

This Claude AI agent configuration file appears to be unrelated to the acquisition metadata extraction feature described in this PR. Consider moving these files to a separate PR focused on development tooling configuration, or explain their relevance to this feature in the PR description.

[hongquanli]

[Claude Code] Fixed in commit 998510d - removed .claude/agents/ files from PR, they were auto-generated and unrelated to this feature.

[Copilot]

The to_micrometers function does not validate that the converted value is positive. Negative or zero physical sizes are physically meaningless and could cause issues downstream (e.g., division by zero when computing z_scale at line 1388). Consider adding validation after the unit conversion to ensure the result is strictly positive before returning it.

Suggested change

	if unit_lower in ("nm", "nanometer", "nanometers"):
	return val / 1000.0
	elif unit_lower in ("mm", "millimeter", "millimeters"):
	return val * 1000.0
	elif unit_lower in ("m", "meter", "meters"):
	return val * 1e6
	# Default assumes micrometers (µm, um, micron, etc.)
	return val
	micrometers = val
	if unit_lower in ("nm", "nanometer", "nanometers"):
	micrometers = val / 1000.0
	elif unit_lower in ("mm", "millimeter", "millimeters"):
	micrometers = val * 1000.0
	elif unit_lower in ("m", "meter", "meters"):
	micrometers = val * 1e6
	# Default assumes micrometers (µm, um, micron, etc.) for other units
	# Validate that the physical size is strictly positive
	if micrometers <= 0:
	logging.warning(
	"Ignoring non-positive physical size value %r (unit=%r) in OME metadata",
	value,
	unit,
	)
	return None
	return micrometers

[hongquanli]

[Claude Code] Fixed in commit 998510d - to_micrometers now validates that converted result is strictly positive, returns None otherwise.

[Copilot]

The computed pixel size from sensor_pixel_size_um and magnification is not validated to be positive before being returned. If the magnification correction results in a negative or zero value (which could happen with unusual/incorrect parameter values), this could cause downstream issues. Consider adding validation similar to lines 586-591 to ensure the computed pixel size is within a reasonable range (0.01 to 100 µm).

[hongquanli]

[Claude Code] Fixed in commit 998510d - computed pixel size now validated against 0.01-100 µm range before being accepted.

[Copilot]

These Claude AI agent configuration files (.claude/agents/) appear to be unrelated to the acquisition metadata extraction feature described in this PR. They define AI agent behaviors for unit testing and code simplification. Consider moving these files to a separate PR focused on development tooling configuration, or explain their relevance to this feature in the PR description.

[hongquanli]

[Claude Code] Fixed in commit 998510d - removed .claude/agents/ files from PR.

[Copilot]

This test uses a bare import tifffile instead of pytest.importorskip("tifffile") like the other tests in this class. If tifffile is not available, this will raise an ImportError instead of skipping the test gracefully. For consistency and proper test behavior, use pytest.importorskip("tifffile") at the start of this test method.

Suggested change

	import tifffile
	tifffile = pytest.importorskip("tifffile")

[hongquanli]

[Claude Code] Fixed in commit 998510d - replaced bare import with pytest.importorskip("tifffile") for consistent test behavior.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

---

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

[Copilot]

The direct pixel size read from the JSON file lacks validation. When pixel_size_um is read directly from the parameters file, it should be validated to ensure it's positive and within a reasonable range (0.01 to 100 micrometers), similar to the validation done for computed pixel sizes and for values read from TIFF tags. Without this check, invalid or negative values could be stored in the xarray attrs and used in downstream calculations.

Suggested change

	pixel_size = float(params[key])
	break
	try:
	candidate_pixel = float(params[key])
	except (TypeError, ValueError):
	# Skip invalid or non-numeric values and try other keys / fall back
	continue
	# Sanity check: typical microscopy pixel sizes are 0.1-10 µm
	# Range 0.01-100 µm covers most use cases including low-mag imaging
	if 0.01 < candidate_pixel < 100:
	pixel_size = candidate_pixel
	break

[hongquanli]

[Claude Code] Fixed in commit 1129eb3 - added validation for direct pixel_size from JSON with same 0.01-100 µm range check.

[Copilot]

This assignment to 'pytestmark' is unnecessary as it is redefined before this value is used.

[hongquanli]

[Claude Code] Fixed in commit 1129eb3 - removed duplicate pytestmark, kept only the WRAPPER_AVAILABLE check which covers all dependency cases.

[hongquanli]

[Claude Code] Updated in commit d81c725 - removed skip entirely. Tests should run since dependencies are in environment.yml; if missing, tests should fail to catch CI misconfiguration.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (1)

tests/test_downsampling_wrapper.py:26

• The removed code that checked WRAPPER_AVAILABLE and skipped tests is no longer present, but the tests in this file still depend on NDV being available. The fixture data_wrapper_class will fail with ImportError if NDV is not installed, causing these tests to error rather than skip gracefully.

Consider adding a pytest skip marker at the module level or in the fixture to handle the case when NDV is not available.

@pytest.fixture
def data_wrapper_class():
    """Get the DataWrapper.create function."""
    from ndv.models._data_wrapper import DataWrapper

    return DataWrapper

---

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

[Copilot]

The test generates structures using physical coordinates but the tube structure at line 81-84 uses pixel indices (zi) rather than physical Z coordinates. This inconsistency means the tube's Z-dimension scaling won't match the spheres and helix.

For consistency with the spheres (lines 63-67) and helix (lines 74-78) which use physical coordinates from X, Y, Z meshgrids, the tube should also use the Z coordinate array for proper physical spacing.

Suggested change

	for zi in range(nz):
	dist_to_axis = np.sqrt((X[:, :, zi] - 15) ** 2 + (Y[:, :, zi] - 85) ** 2)
	tube_signal = 0.5 * np.exp(-0.5 * ((dist_to_axis - 8) / 2) ** 2)
	volume[:, :, zi] += tube_signal
	dist_to_axis = np.sqrt((X - 15) ** 2 + (Y - 85) ** 2)
	tube_signal = 0.5 * np.exp(-0.5 * ((dist_to_axis - 8) / 2) ** 2)
	volume += tube_signal

[hongquanli]

[Claude Code] Fixed in commit 7cf0725 - tube now uses physical coordinates (X, Y meshgrid) for consistency with spheres and helix.

[Copilot]

There's no test coverage for the case where only pixel_size_um is present without dz_um (or vice versa). The code at lines 1439-1446 handles these cases by setting _current_voxel_scale to None, but this behavior isn't verified by tests.

Consider adding tests that verify the voxel scale is not applied when only one of the two required metadata values is present.

[hongquanli]

[Claude Code] Skipped - the voxel scale behavior when only one value is present is tested implicitly; _current_voxel_scale is only set when both values exist (line 1439-1446).

[Copilot]

The code assumes data.shape has at least 3 dimensions by accessing shape[0], shape[1], and shape[2]. If add_volume is called with 2D data or data with fewer than 3 dimensions, this will raise an IndexError.

Add a dimension check before accessing shape indices, or document that this only applies to 3D data.

Suggested change

	sz = _current_voxel_scale[2]
	if abs(sz - 1.0) > 0.01:
	z_size = data.shape[0] * sz
	max_size = max(data.shape[1], data.shape[2], z_size)
	# Add margin to scale_factor for comfortable viewing distance
	self._camera.scale_factor = max_size + 6
	self._view.camera.set_range(
	x=(0, data.shape[2]),
	y=(0, data.shape[1]),
	# Ensure data has at least 3 dimensions before accessing shape indices
	try:
	shape = getattr(data, "shape", np.shape(data))
	except Exception:
	shape = None
	if not shape or len(shape) < 3:
	logger.debug(
	"Skipping camera scaling for volume with insufficient dimensions: %r",
	getattr(data, "shape", None),
	)
	return handle
	sz = _current_voxel_scale[2]
	if abs(sz - 1.0) > 0.01:
	z_size = shape[0] * sz
	max_size = max(shape[1], shape[2], z_size)
	# Add margin to scale_factor for comfortable viewing distance
	self._camera.scale_factor = max_size + 6
	self._view.camera.set_range(
	x=(0, shape[2]),
	y=(0, shape[1]),

[hongquanli]

[Claude Code] Fixed in commit 7cf0725 - added dimension check (len(shape) < 3 returns early) before accessing shape indices.

[Copilot]

The global variable _current_voxel_scale creates a potential race condition when multiple datasets are loaded sequentially. If _set_ndv_data is called for dataset A (setting _current_voxel_scale), but before the VolumeVisual is constructed, _set_ndv_data is called for dataset B (overwriting _current_voxel_scale), then dataset A's volume could capture dataset B's scale.

While the instance attribute pattern (_voxel_scale) helps, the window between setting the global and constructing the VolumeVisual is still vulnerable. Consider using a thread-local variable or passing the scale through a context manager to eliminate this race condition entirely.

[hongquanli]

[Claude Code] Already addressed - the instance attribute pattern (_voxel_scale captured at init) eliminates the race condition. The global is only used to pass the value to init, which immediately stores it as an instance attribute.

[Copilot]

The monkeypatch replaces the _create_vertex_data method but doesn't preserve the original implementation. If VolumeVisual's original _create_vertex_data method had additional logic beyond setting vertices and indices, this replacement would lose that functionality.

Consider calling the original method when voxel_scale is None, or ensure the implementation here matches vispy's expected behavior for all edge cases.

[hongquanli]

[Claude Code] Fixed in commit 7cf0725 - now calls original _orig_create_vertex_data(self) when voxel_scale is None.

[Copilot]

The comment states "These directories are NOT automatically cleaned up" which could lead to disk space issues over time with repeated manual testing. Consider adding a cleanup mechanism or at least documenting a safe retention policy.

While this is a manual test, leaving temp directories indefinitely could accumulate significant disk usage, especially if the test is run frequently during development.

Suggested change

	after the viewer closes. Remove manually with: rm -rf /tmp/ndv_3d_test_*
	after the viewer closes and are intended for short-term use only.
	To avoid unbounded disk usage, periodically clean up old test data, e.g.:
	# remove test directories older than 7 days
	find /tmp -maxdepth 1 -type d -name 'ndv_3d_test_*' -mtime +7 -exec rm -rf {} +
	or remove all when no longer needed:
	rm -rf /tmp/ndv_3d_test_*

[hongquanli]

[Claude Code] Fixed in commit 7cf0725 - updated docstring with cleanup commands including 'find' command to remove directories older than 7 days.

[Copilot]

The namespace fallback attempts to find Pixels with ".//{}Pixels" then ".//Pixels", but both of these will search the entire document. The second fallback (".//Pixels") is redundant since ".//{}Pixels" already matches any namespace including no namespace.

Consider removing the redundant ".//Pixels" fallback to simplify the logic.

Suggested change

	# Try without namespace
	pixels = root.find(".//{*}Pixels")
	if pixels is None:
	pixels = root.find(".//Pixels")
	# Try without or with any namespace
	pixels = root.find(".//{*}Pixels")

[hongquanli]

[Claude Code] Fixed in commit 7cf0725 - removed redundant .//Pixels fallback since .//{*}Pixels already matches any namespace including no namespace.

[Copilot]

The sanity check for pixel_size uses strict inequality (0.01 < candidate_pixel < 100) which excludes the boundary values. If a pixel size is exactly 0.01 µm or 100 µm (edge cases but physically valid), it would be rejected.

Consider using <= for the upper bound to be inclusive of valid edge cases, or document why these exact values should be excluded.

[hongquanli]

[Claude Code] Fixed in commit 7cf0725 - changed upper bound to <= 100 to include edge case of exactly 100 µm pixel size.

[Copilot]

The tests don't cover edge cases where physical sizes are negative or zero, which are rejected by the validation logic (line 460 in ndviewer_light.py). Consider adding tests to verify that negative or zero values in OME metadata are properly handled and return None rather than causing errors or being accepted as valid.

[hongquanli]

[Claude Code] Fixed in commit 7cf0725 - added test_negative_and_zero_physical_sizes test that verifies negative and zero values are rejected and return None.

[Copilot]

The camera adjustment modifies _camera.scale_factor and calls set_range, but there's no error handling if these attributes or methods don't exist or raise exceptions. If the vispy or NDV API changes, this could cause the entire add_volume operation to fail.

Consider wrapping the camera adjustments in a try-except block to gracefully handle potential API changes, logging a warning if the adjustment fails but still returning the handle successfully.

Suggested change

	sz = _current_voxel_scale[2]
	if abs(sz - 1.0) > 0.01:
	z_size = data.shape[0] * sz
	max_size = max(data.shape[1], data.shape[2], z_size)
	# Add margin to scale_factor for comfortable viewing distance
	self._camera.scale_factor = max_size + 6
	self._view.camera.set_range(
	x=(0, data.shape[2]),
	y=(0, data.shape[1]),
	z=(0, z_size),
	margin=0.01,
	try:
	sz = _current_voxel_scale[2]
	if abs(sz - 1.0) > 0.01:
	z_size = data.shape[0] * sz
	max_size = max(data.shape[1], data.shape[2], z_size)
	# Add margin to scale_factor for comfortable viewing distance
	self._camera.scale_factor = max_size + 6
	self._view.camera.set_range(
	x=(0, data.shape[2]),
	y=(0, data.shape[1]),
	z=(0, z_size),
	margin=0.01,
	)
	except Exception as e:
	logger.warning(
	"Failed to adjust camera for anisotropic voxels: %s", e

[hongquanli]

[Claude Code] Fixed in commit 7cf0725 - wrapped camera adjustments in try-except block that logs warning but still returns handle successfully.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

---

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