Repository navigation
Adding Scale from Metadata to Layer - #3
Conversation
added _get_scales function to get scale factors for zyx dimensions
Changed key "scales" to "scale" in metadata dict
Scale z stack
There was a problem hiding this comment.
Pull request overview
Adds extraction of physical pixel sizes from BioIO metadata and attempts to expose them as napari layer scaling information.
Changes:
- Introduces an internal helper (
_get_scale) to read Z/Y/X physical pixel sizes from metadata. - Adds a
scalefield to the returned napari layer kwargs/metadata for single- and multi-scene reads.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| meta = { | ||
| "name": layer_name, | ||
| "metadata": { | ||
| "bioio_metadata": img.metadata, | ||
| "scene_info": { | ||
| "scene_id": scene_id, | ||
| "scene_index": scene_idx, | ||
| "scene_name": scene_name, | ||
| "total_scenes": len(available_scenes), | ||
| "scale": _get_scale(img.metadata) | ||
|
|
||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
scale is attached inconsistently: for single-scene it is a top-level layer kwarg (meta['scale']), but for multi-scene it is nested under meta['metadata']['scene_info']['scale']. Napari only looks for scale as a top-level layer kwarg, and the PR description says it should be under meta['metadata']. Pick one placement and apply it consistently (prefer the top-level scale kwarg if the goal is to affect layer scaling).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if len(available_scenes) == 1: | ||
| data = img.data | ||
| meta = {"name": base_name, "metadata": img.metadata} | ||
| meta = {"name": base_name, "metadata": img.metadata, "scale": _get_scale(img.metadata)} |
There was a problem hiding this comment.
New scale behavior isn’t covered by tests. Since this repo already has reader tests, add assertions that returned layer metadata includes scale when physical sizes are present and that its length matches data.ndim (including for the TCZYX synthetic TIFF in test_reader_multidimensional_tiff).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
Hi @M-A-Demir , |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| meta = { | ||
| "name": layer_name, | ||
| "metadata": { | ||
| "bioio_metadata": img.metadata, | ||
| "scene_info": { | ||
| "scene_id": scene_id, | ||
| "scene_index": scene_idx, | ||
| "scene_name": scene_name, | ||
| "total_scenes": len(available_scenes), | ||
| "scale": _get_scale(img.metadata) | ||
|
|
||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
scale is attached inconsistently and (for multi-scene) in a place napari won’t use for layer scaling: single-scene sets top-level meta['scale'], while multi-scene stores it under meta['metadata']['scene_info']['scale']. The PR description also says it should be under meta['metadata']['scale']. Please pick a single, consistent placement (and if the intent is to scale the layer, ensure scale is provided as the top-level layer kwarg for both single- and multi-scene paths).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| # Construct a full-length scale vector for napari: | ||
| # pad non-spatial axes (e.g., T, C) with 1.0 and apply spatial scales to the last axes. | ||
| scale = _get_scale(img.metadata) | ||
| try: | ||
| # Treat scale as a sequence; napari expects len(scale) == data.ndim | ||
| scale_len = len(scale) # type: ignore[arg-type] | ||
| except TypeError: | ||
| # If scale is a scalar, leave it as-is | ||
| full_scale = scale | ||
| else: | ||
| if scale_len == data.ndim: | ||
| full_scale = scale | ||
| elif scale_len < data.ndim: | ||
| # Assume spatial dimensions are the last axes | ||
| pad_len = data.ndim - scale_len | ||
| full_scale = [1.0] * pad_len + list(scale) | ||
| else: | ||
| # More scales than dimensions: truncate to match data.ndim | ||
| full_scale = list(scale)[-data.ndim :] | ||
|
|
||
| meta = { | ||
| "name": base_name, | ||
| "metadata": img.metadata, | ||
| "scale": full_scale, | ||
| } |
There was a problem hiding this comment.
New scale behavior isn’t covered by tests. This repo already has pytest coverage for the reader (e.g., test_reader_multidimensional_tiff), so add assertions that (1) scale is present when physical pixel sizes are available, and (2) the provided scale has length data.ndim (including padding for non-spatial axes like T/C).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| meta = { | ||
| "name": base_name, | ||
| "metadata": img.metadata, | ||
| "scale": full_scale, | ||
| } |
There was a problem hiding this comment.
The PR description says scale should be appended under meta['metadata']['scale'], but the single-scene path sets it as a top-level layer kwarg (meta['scale']). Align the implementation with the documented behavior (or update the description) and keep placement consistent across single- and multi-scene layers.
| meta = { | ||
| "name": layer_name, | ||
| "metadata": { | ||
| "bioio_metadata": img.metadata, | ||
| "scene_info": { | ||
| "scene_id": scene_id, | ||
| "scene_index": scene_idx, | ||
| "scene_name": scene_name, | ||
| "total_scenes": len(available_scenes), | ||
| "scale": _get_scale(img.metadata) | ||
| }, |
There was a problem hiding this comment.
In the multi-scene branch, scale is stored under meta['metadata']['scene_info']['scale'] rather than as the top-level meta['scale'] that napari uses for layer scaling. This also makes scale placement inconsistent across the single- vs multi-scene paths. Provide scale as a top-level layer kwarg for each scene layer (and keep any nested copy if needed).
| "scene_index": scene_idx, | ||
| "scene_name": scene_name, | ||
| "total_scenes": len(available_scenes), | ||
| "scale": _get_scale(img.metadata) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Multi-scene layers don’t apply the same padding/truncation logic used in the single-scene path to ensure len(scale) == data.ndim. As written, _get_scale may return 1–3 values while data may be TCZYX, which can cause napari to reject the layer kwargs or apply incorrect scaling. Reuse the same full_scale construction for multi-scene layers.
| try: | ||
| # Treat scale as a sequence; napari expects len(scale) == data.ndim | ||
| scale_len = len(scale) # type: ignore[arg-type] | ||
| except TypeError: | ||
| # If scale is a scalar, leave it as-is | ||
| full_scale = scale | ||
| else: | ||
| if scale_len == data.ndim: | ||
| full_scale = scale | ||
| elif scale_len < data.ndim: | ||
| # Assume spatial dimensions are the last axes | ||
| pad_len = data.ndim - scale_len | ||
| full_scale = [1.0] * pad_len + list(scale) | ||
| else: | ||
| # More scales than dimensions: truncate to match data.ndim | ||
| full_scale = list(scale)[-data.ndim :] | ||
|
|
||
| meta = { | ||
| "name": base_name, | ||
| "metadata": img.metadata, | ||
| "scale": full_scale, | ||
| } |
There was a problem hiding this comment.
When _get_scale returns None, the single-scene path sets meta['scale'] = None. Napari layer kwargs generally expect a numeric scalar or a sequence; passing None may raise at layer creation time. Consider omitting the scale key entirely when no scale is available (or defaulting to [1.0] * data.ndim).
| try: | |
| # Treat scale as a sequence; napari expects len(scale) == data.ndim | |
| scale_len = len(scale) # type: ignore[arg-type] | |
| except TypeError: | |
| # If scale is a scalar, leave it as-is | |
| full_scale = scale | |
| else: | |
| if scale_len == data.ndim: | |
| full_scale = scale | |
| elif scale_len < data.ndim: | |
| # Assume spatial dimensions are the last axes | |
| pad_len = data.ndim - scale_len | |
| full_scale = [1.0] * pad_len + list(scale) | |
| else: | |
| # More scales than dimensions: truncate to match data.ndim | |
| full_scale = list(scale)[-data.ndim :] | |
| meta = { | |
| "name": base_name, | |
| "metadata": img.metadata, | |
| "scale": full_scale, | |
| } | |
| full_scale = None | |
| if scale is not None: | |
| try: | |
| # Treat scale as a sequence; napari expects len(scale) == data.ndim | |
| scale_len = len(scale) # type: ignore[arg-type] | |
| except TypeError: | |
| # If scale is a scalar, leave it as-is | |
| full_scale = scale | |
| else: | |
| if scale_len == data.ndim: | |
| full_scale = scale | |
| elif scale_len < data.ndim: | |
| # Assume spatial dimensions are the last axes | |
| pad_len = data.ndim - scale_len | |
| full_scale = [1.0] * pad_len + list(scale) | |
| else: | |
| # More scales than dimensions: truncate to match data.ndim | |
| full_scale = list(scale)[-data.ndim :] | |
| meta = { | |
| "name": base_name, | |
| "metadata": img.metadata, | |
| } | |
| if full_scale is not None: | |
| meta["scale"] = full_scale |
| # Construct a full-length scale vector for napari: | ||
| # pad non-spatial axes (e.g., T, C) with 1.0 and apply spatial scales to the last axes. | ||
| scale = _get_scale(img.metadata) | ||
| try: | ||
| # Treat scale as a sequence; napari expects len(scale) == data.ndim | ||
| scale_len = len(scale) # type: ignore[arg-type] | ||
| except TypeError: | ||
| # If scale is a scalar, leave it as-is | ||
| full_scale = scale | ||
| else: | ||
| if scale_len == data.ndim: | ||
| full_scale = scale | ||
| elif scale_len < data.ndim: | ||
| # Assume spatial dimensions are the last axes | ||
| pad_len = data.ndim - scale_len | ||
| full_scale = [1.0] * pad_len + list(scale) | ||
| else: | ||
| # More scales than dimensions: truncate to match data.ndim | ||
| full_scale = list(scale)[-data.ndim :] | ||
|
|
||
| meta = { | ||
| "name": base_name, | ||
| "metadata": img.metadata, | ||
| "scale": full_scale, | ||
| } | ||
| layers.append((data, meta, "image")) |
There was a problem hiding this comment.
The new scale behavior isn’t covered by existing pytest reader tests (src/napari_bioio_reader/_tests/test_reader.py). Add assertions that (1) layers include a scale kwarg when physical sizes are present, and (2) the provided scale length matches data.ndim (including padding for non-spatial axes like T/C).
| "scale": full_scale, | ||
| } | ||
| layers.append((data, meta, "image")) | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace on this blank line; please remove it to keep diffs clean and avoid style-only churn in future changes.
| A list of available physical pixel sizes in ``[z, y, x]`` order. | ||
| Only axes with a defined physical size are included, so the list | ||
| may have length 1–3 (e.g. ``[y, x]`` or ``[x]``). Returns ``None`` | ||
| if no physical pixel sizes can be determined. | ||
| """ | ||
| scale = [] | ||
| try: | ||
| images = getattr(metadata, "images", None) | ||
| if not images: | ||
| return None | ||
|
|
||
| first_image = images[0] | ||
| px_data = getattr(first_image, "pixels", None) | ||
| if px_data is None: | ||
| return None | ||
|
|
||
| if getattr(px_data, "physical_size_z", None) is not None: | ||
| scale.append(px_data.physical_size_z) | ||
| if getattr(px_data, "physical_size_y", None) is not None: | ||
| scale.append(px_data.physical_size_y) | ||
| if getattr(px_data, "physical_size_x", None) is not None: | ||
| scale.append(px_data.physical_size_x) |
There was a problem hiding this comment.
_get_scale drops missing axes and returns a variable-length list, but later code assumes the returned values map to the last axes of data. If Z is defined but Y is missing (possible with current logic), _get_scale returns [z, x] and the padding logic will incorrectly align z with Y. Preserve axis positions by returning a fixed-length [z, y, x] (using 1.0/None placeholders) or a dict keyed by axis, then build full_scale from explicit axis mapping.
Hi @lguerard! No problem at all, I'll run a couple of quick checks by the end of this week. Thank you very much! |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
Thanks a lot ! And sorry for the whole mess here with the comments, I'm trying things with copilot in PR, not yet super convinced 😅 |
|
Hello again @lguerard! I installed this update and opened the .nd2 file that required a scale in it, it looks good! I also used the plugin to open a non-scaled .nd2 file, and a .tiff, (just to make sure nothing broke) and that seems fine too. Thanks for your help on this:) |
|
Thanks a lot for testing ! I think I'll merge and if there are any issues later, then it's going to be fixed then ;) |
Added ability to obtain scaling info for zyx coords, and append them to a dict key called "scale" in the meta["metadata"] dictionary. New function: _get_scale.