-
Notifications
You must be signed in to change notification settings - Fork 1
Add reverse_coords option to point conversion functions #15
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for fixing the order of point coordinates, @thewtex
I find some parts of this pull request (adjusting ".gitignore" and adding/removing binaries for Python 3.12) hard to review. Are they essential for the "reverse_coords" fix? If so, can you place them in a separate commit? If they are unrelated, please move them into a separate pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@N-Dekker I'll get this published soon to ease your application in elastix-napari.
I've cleaned up the history and added the .gitignore changes to a new PR.
| data = points_array | ||
| if reverse_coords: | ||
| # Reverse coordinate order from ITK (x,y,z) to napari (z,y,x) | ||
| data = points_array[:, ::-1] | ||
| else: | ||
| data = points_array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main part of the fix, right? Cool! I can't wait to try it out, with SuperElastix/elastix-napari#65 😃
| if reverse_coords: | ||
| # Reverse coordinate order from napari (z,y,x) to ITK (x,y,z) | ||
| data = data[:, ::-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, the other "main" part of the fix 😃
|
@thewtex For my understanding, is this proposed new In practice, shouldn't the coordinates always be reverted anyway, when converting between a napari Points layer and an |
- Add reverse_coords boolean kwarg (default True) to points_layer_from_point_set - Add reverse_coords boolean kwarg (default True) to point_set_from_points_layer - When enabled, converts between ITK's x,y,z order and napari's z,y,x order - Update existing tests to account for new default behavior - Add tests for reverse_coords=False, roundtrip, and 2D points Co-authored-by: thewtex <[email protected]>
a051f95 to
d5e5a73
Compare
@N-Dekker yes, it will be permanent. It is left as an option in case the client calling the function already made the change (using with a different library, etc.) to avoid the need and performance hit for doing it again. |
|
@N-Dekker published to PyPI as version 0.5.1 |
Including pull request InsightSoftwareConsortium/itk-napari-conversion#15 commit InsightSoftwareConsortium/itk-napari-conversion@d5e5a73 "Add reverse_coords option to point conversion functions"
Including pull request InsightSoftwareConsortium/itk-napari-conversion#15 commit InsightSoftwareConsortium/itk-napari-conversion@d5e5a73 "Add reverse_coords option to point conversion functions"
ITK uses x,y,z coordinate order while napari uses z,y,x (row-major NumPy). The point conversion functions now handle this automatically via a
reverse_coordskwarg, enabled by default.Changes
points_layer_from_point_set: Addedreverse_coords=Truekwarg to reverse ITK (x,y,z) → napari (z,y,x)point_set_from_points_layer: Addedreverse_coords=Truekwarg to reverse napari (z,y,x) → ITK (x,y,z)reverse_coords=False, roundtrip conversions, and 2D pointsUsage
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.