Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for the get_transform analog to set_transform #264

Merged
merged 13 commits into from
Dec 20, 2024

Conversation

pattonw
Copy link
Contributor

@pattonw pattonw commented Dec 10, 2024

given a set of scale and translation transforms, defined on both the group and individual individual images, each image will have a final scale and translation. These can now be conveniently retrieved with a little helper function that fetches all appropriate translations and applies them consecutively.

small test added to cover changes.

This reverts the formatting changes that were accidentally included with commit  0c21a73.
@ziw-liu ziw-liu added enhancement New feature or request NGFF OME-NGFF (OME-Zarr format) labels Dec 10, 2024
Copy link
Collaborator

@ziw-liu ziw-liu 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 your contribution! I've left my comments inline.

iohub/ngff/nodes.py Outdated Show resolved Hide resolved
iohub/ngff/nodes.py Outdated Show resolved Hide resolved
iohub/ngff/nodes.py Outdated Show resolved Hide resolved
@ziw-liu ziw-liu added this to the 0.2.0 milestone Dec 10, 2024
@ziw-liu
Copy link
Collaborator

ziw-liu commented Dec 10, 2024

Re: CI failure: please refer to the contributing guide.

@pattonw
Copy link
Contributor Author

pattonw commented Dec 10, 2024

passed the precommit checks 👍

iohub/ngff/nodes.py Outdated Show resolved Hide resolved
iohub/ngff/nodes.py Outdated Show resolved Hide resolved
iohub/ngff/nodes.py Show resolved Hide resolved
tests/ngff/test_ngff.py Outdated Show resolved Hide resolved
Copy link
Member

@JoOkuma JoOkuma left a comment

Choose a reason for hiding this comment

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

@ziw-liu, If get_effective_translation[or scale] returns a single transformation object would it be more friendly to return a tuple of floats or an array-like object?

@pattonw, I agree with the other comments from @ziw-liu

@ziw-liu
Copy link
Collaborator

ziw-liu commented Dec 11, 2024

If get_effective_translation[or scale] returns a single transformation object would it be more friendly to return a tuple of floats or an array-like object?

If the dataset does not have all 5 axes, what should the length of the returned sequence be? I'm guessing that @pattonw's use cases are mostly 3D instead of 5D?

Edit: I remembered it wrong

@pattonw
Copy link
Contributor Author

pattonw commented Dec 11, 2024

If get_effective_translation[or scale] returns a single transformation object would it be more friendly to return a tuple of floats or an array-like object?

If the dataset does not have all 5 axes, what should the length of the returned sequence be? I'm guessing that @pattonw's use cases are mostly 3D instead of 5D?

Isn't it as simple as returning the .scale or .translation property of the TransformationMeta? Isn't the length always equal to the number of dimensions in the array?

@ziw-liu
Copy link
Collaborator

ziw-liu commented Dec 11, 2024

Isn't it as simple as returning the .scale or .translation property of the TransformationMeta? Isn't the length always equal to the number of dimensions in the array?

You're right! I mistakenly thought that Position.scale behaves differently (I was thinking of this one).

iohub/ngff/nodes.py Outdated Show resolved Hide resolved
@pattonw
Copy link
Contributor Author

pattonw commented Dec 11, 2024

Separated into 2 unit tests, return plain list of floats instead of TransformationMeta classes, and removed the error I introduced in scale property to fetch the effective scale of the first dataset instead of just the root.

tests/ngff/test_ngff.py Outdated Show resolved Hide resolved
tests/ngff/test_ngff.py Outdated Show resolved Hide resolved
@pattonw
Copy link
Contributor Author

pattonw commented Dec 19, 2024

fixed test docstring and fixed case that was failing testing

Copy link
Collaborator

@ziw-liu ziw-liu left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @pattonw for your contribution!

@ziw-liu
Copy link
Collaborator

ziw-liu commented Dec 19, 2024

Ready to merge if @JoOkuma doen't have further comments.

@JoOkuma
Copy link
Member

JoOkuma commented Dec 19, 2024

@ziw-liu, I don't have any additional comments.
Thanks for the contribution @pattonw

@ziw-liu ziw-liu merged commit c8822bc into czbiohub-sf:main Dec 20, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request NGFF OME-NGFF (OME-Zarr format)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants