Skip to content

Set scale for any coordinate transform#287

Merged
ziw-liu merged 4 commits into
mainfrom
fix-set-scale-level
Apr 2, 2025
Merged

Set scale for any coordinate transform#287
ziw-liu merged 4 commits into
mainfrom
fix-set-scale-level

Conversation

@ziw-liu
Copy link
Copy Markdown
Contributor

@ziw-liu ziw-liu commented Mar 29, 2025

Fix #278.

Position.set_scale now works for any level, like Position.set_transform.

@ziw-liu ziw-liu added this to the 0.2.0 milestone Mar 29, 2025
@ziw-liu ziw-liu marked this pull request as ready for review March 29, 2025 00:40
Copy link
Copy Markdown
Contributor

@ieivanov ieivanov left a comment

Choose a reason for hiding this comment

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

This looks good to me, it's best if @talonchandler also takes a look before merging.

@ziw-liu
Copy link
Copy Markdown
Contributor Author

ziw-liu commented Apr 1, 2025

Also @edyoshikun you asked for the organelle box dataset.

Comment thread iohub/ngff/nodes.py
Copy link
Copy Markdown
Collaborator

@edyoshikun edyoshikun left a comment

Choose a reason for hiding this comment

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

The comments above are not a dealbreaker.

LGTM! Tested the CLI and the script after bumping my environment to 3.11

Comment thread iohub/ngff/nodes.py
@edyoshikun
Copy link
Copy Markdown
Collaborator

@ziw-liu I just saw your comments. this all LGTM! thanks

Copy link
Copy Markdown
Contributor

@talonchandler talonchandler left a comment

Choose a reason for hiding this comment

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

Thanks for generalizing this @ziw-liu. I tested on a pyramid, and all is working as expected.

My only non-blocking suggestion is to consider renaming image to multiscale-level or similar. My understanding of the OME-Zarr spec is that they refer to the whole pyramid as an image, and they refer to each level of the pyramid as a multiscale-level. IMO this renaming would also make it easier to understand what this option is for.

Comment thread iohub/cli/cli.py Outdated
Co-authored-by: Talon Chandler <talonchandler@gmail.com>
@ziw-liu
Copy link
Copy Markdown
Contributor Author

ziw-liu commented Apr 2, 2025

My only non-blocking suggestion is to consider renaming image to multiscale-level or similar

This argument name is consistent with iohub.ngff.ImageArray. Probably not the best type naming but that would be a breaking change.

@ziw-liu ziw-liu merged commit 038f378 into main Apr 2, 2025
7 checks passed
@ziw-liu ziw-liu deleted the fix-set-scale-level branch April 2, 2025 18:01
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.

Set-scale is hard-coded to the highest resolution

4 participants