ENH: Remove docstrings on itk.Image axis ordering. Remove ANTS option.#66
Merged
Merged
Conversation
Remove the project guideline requiring axis order and shape to be stated in every docstring and comment touching arrays. ITK's (Z, Y, X) numpy convention is well-known and was adding noise without value. Comments for non-standard orderings (pynrrd, ANTs vector images) are kept. Apply the same policy to source files: drop "Axis ordering:" subsections and inline (Z, Y, X) / (X, Y, Z) shape annotations from register_images_icon.py, image_tools.py, labelmap_tools.py, contour_tools.py, register_images_ants.py, register_images_greedy.py, convert_image_4d_to_3d.py, transform_tools.py, and test_tools.py. Fix all callers that passed legacy method-name strings to RegisterTimeSeriesImages: - "ANTS" / "ANTS_ICON" → "Greedy" / "Greedy_ICON" in tests, tutorial_08_dirlab_pca_time_series.py, and experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py - "greedy" (lowercase) → "Greedy" in tests - set_number_of_iterations_ANTS() → set_number_of_iterations_greedy() at all affected call sites - Repurpose test_registrar_initialization_ANTS as test_registrar_initialization_Greedy_ICON to cover the previously untested combined pipeline RegisterImagesANTS is unchanged; REGISTRATION_METHODS stays ["Greedy", "ICON", "Greedy_ICON"]. Baselines for affected slow tests will need regeneration.
|
Warning Review limit reached
More reviews will be available in 35 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
WalkthroughThis PR simultaneously simplifies documentation and migrates the registration algorithm. Contributor guidelines and image tool docstrings throughout the codebase remove explicit axis-ordering explanations, while registration configuration and tests switch from ANTs to Greedy as the primary algorithm. ChangesDocumentation Simplification and Algorithm Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR removes the repository guideline of repeatedly documenting ITK↔NumPy axis ordering in docstrings/comments, and updates time-series registration call sites to drop legacy ANTs/ANTS_ICON method strings in favor of the supported Greedy/ICON variants.
Changes:
- Removes pervasive “Axis ordering:” sections and inline (Z,Y,X)/(X,Y,Z) annotations across multiple modules to reduce documentation noise.
- Updates tutorials/experiments/tests to use
RegisterTimeSeriesImagesregistration methodsGreedy,ICON, andGreedy_ICON(and updates iteration setter call sites accordingly). - Removes the “state axis order and shape everywhere” guidance from
CLAUDE.mdandAGENTS.md.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_08_dirlab_pca_time_series.py | Switches time-series registration method from legacy ANTS_ICON to Greedy_ICON. |
| tests/test_register_time_series_images.py | Updates tests to use Greedy/ICON method names and the Greedy iterations setter. |
| src/physiomotion4d/transform_tools.py | Removes axis-order inline comments around displacement field NumPy usage. |
| src/physiomotion4d/test_tools.py | Removes detailed ITK array axis-order explanation from screenshot helper docstring. |
| src/physiomotion4d/register_images_icon.py | Trims detailed axis-order exposition in ICON preprocessing docstrings. |
| src/physiomotion4d/register_images_greedy.py | Removes axis-order mention in Greedy helper docstrings. |
| src/physiomotion4d/register_images_ants.py | Removes a vector-image axis-order comment in ANTs conversion helper. |
| src/physiomotion4d/labelmap_tools.py | Removes axis-order subsections from labelmap/mask distance-map docstrings. |
| src/physiomotion4d/image_tools.py | Removes axis-order notes/comments from ITK↔SimpleITK conversion docstrings. |
| src/physiomotion4d/convert_image_4d_to_3d.py | Removes explicit axis-order/shape mentions from class/method docstrings. |
| src/physiomotion4d/contour_tools.py | Simplifies transpose commentary during trimesh voxelization → ITK image creation. |
| experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py | Updates registration method lists and iteration setter calls to Greedy equivalents. |
| CLAUDE.md | Removes the guideline requiring explicit axis-order/shape in every array-related doc/comment. |
| AGENTS.md | Removes the same guideline from agent-facing documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
152
to
156
| USD `+X=Left`, `+Y=Superior`, `+Z=Anterior`. | ||
| - Masks are ITK images with integer labels. Keep anatomy group IDs consistent | ||
| across segmenters. | ||
| - Transforms are ITK composite transforms stored in compressed `.hdf` files. | ||
| - State axis order and shape explicitly in every docstring and comment that | ||
| touches arrays. | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py (1)
8-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate comment to reflect Greedy migration.
The comment still mentions "ANTs" but should reference "Greedy" instead, consistent with the algorithm migration throughout this PR.
📝 Suggested fix
-# - Registration of time series images using ANTs, ICON, or ANTs+ICON methods +# - Registration of time series images using Greedy, ICON, or Greedy+ICON methods🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py` at line 8, Update the top-line comment that reads "Registration of time series images using ANTs, ICON, or ANTs+ICON methods" to reference Greedy instead of ANTs (e.g., "Registration of time series images using Greedy, ICON, or Greedy+ICON methods") so it matches the Greedy migration used elsewhere; locate the comment near the reconstruct_4d_ct class/module header and change both plain and combined mentions ("ANTs+ICON") to the corresponding "Greedy" wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py`:
- Around line 65-72: Update the inline comment above the registration
configuration to reflect the active methods: change the comment "Registration
parameters - Greedy and ICON for full run" to indicate only Greedy is enabled
(e.g., "Registration parameters - Greedy for full run") and ensure the
explanatory comment next to number_of_iterations_list aligns (remove or adjust
references to ICON/Greedy_ICON) so the comments match the current
registration_methods and number_of_iterations_list variables.
- Around line 56-58: Update the misleading comment above the registration
configuration so it accurately describes the contents of registration_methods
and number_of_iterations_list: either change the comment to list the three
methods ("Greedy", "ICON", "Greedy_ICON") and note that
number_of_iterations_list corresponds to each method, or reduce
registration_methods to only "Greedy" and adjust number_of_iterations_list
accordingly; refer to the variables registration_methods and
number_of_iterations_list to locate and update the comment.
---
Outside diff comments:
In `@experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py`:
- Line 8: Update the top-line comment that reads "Registration of time series
images using ANTs, ICON, or ANTs+ICON methods" to reference Greedy instead of
ANTs (e.g., "Registration of time series images using Greedy, ICON, or
Greedy+ICON methods") so it matches the Greedy migration used elsewhere; locate
the comment near the reconstruct_4d_ct class/module header and change both plain
and combined mentions ("ANTs+ICON") to the corresponding "Greedy" wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3171e3ee-ec4d-483d-bdf4-a3dfa60e19b0
📒 Files selected for processing (14)
AGENTS.mdCLAUDE.mdexperiments/Reconstruct4DCT/reconstruct_4d_ct_class.pysrc/physiomotion4d/contour_tools.pysrc/physiomotion4d/convert_image_4d_to_3d.pysrc/physiomotion4d/image_tools.pysrc/physiomotion4d/labelmap_tools.pysrc/physiomotion4d/register_images_ants.pysrc/physiomotion4d/register_images_greedy.pysrc/physiomotion4d/register_images_icon.pysrc/physiomotion4d/test_tools.pysrc/physiomotion4d/transform_tools.pytests/test_register_time_series_images.pytutorials/tutorial_08_dirlab_pca_time_series.py
💤 Files with no reviewable changes (7)
- CLAUDE.md
- AGENTS.md
- src/physiomotion4d/image_tools.py
- src/physiomotion4d/labelmap_tools.py
- src/physiomotion4d/test_tools.py
- src/physiomotion4d/register_images_ants.py
- src/physiomotion4d/transform_tools.py
Comment on lines
+56
to
+58
| # Registration parameters - only Greedy for quick run | ||
| registration_methods = ["Greedy", "ICON", "Greedy_ICON"] | ||
| number_of_iterations_list = [[8, 4, 1], 5, [[8, 4, 1], 5]] # For Greedy and ICON |
There was a problem hiding this comment.
Clarify comment to match the actual registration methods list.
The comment says "only Greedy for quick run" but line 57 includes ["Greedy", "ICON", "Greedy_ICON"], which is more than just Greedy.
📝 Suggested fix
- # Registration parameters - only Greedy for quick run
+ # Registration parameters - Greedy-based methods for quick run
registration_methods = ["Greedy", "ICON", "Greedy_ICON"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py` around lines 56 - 58,
Update the misleading comment above the registration configuration so it
accurately describes the contents of registration_methods and
number_of_iterations_list: either change the comment to list the three methods
("Greedy", "ICON", "Greedy_ICON") and note that number_of_iterations_list
corresponds to each method, or reduce registration_methods to only "Greedy" and
adjust number_of_iterations_list accordingly; refer to the variables
registration_methods and number_of_iterations_list to locate and update the
comment.
Comment on lines
+65
to
72
| # Registration parameters - Greedy and ICON for full run | ||
| registration_methods = ["Greedy"] # , "ICON", "Greedy_ICON"] | ||
| number_of_iterations_list = [ | ||
| [30, 15, 7, 3], | ||
| ] # For ANTs | ||
| ] # For Greedy | ||
| # 20, # For ICON | ||
| # [[30, 15, 7, 3], 20], # For ANTS_ICON | ||
| # [[30, 15, 7, 3], 20], # For Greedy_ICON | ||
| # ] |
There was a problem hiding this comment.
Update comment to match the active registration methods.
The comment mentions "Greedy and ICON for full run" but only ["Greedy"] is uncommented on line 66, with ICON and Greedy_ICON commented out.
📝 Suggested fix
- # Registration parameters - Greedy and ICON for full run
+ # Registration parameters - Greedy for full run (ICON options commented out)
registration_methods = ["Greedy"] # , "ICON", "Greedy_ICON"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Registration parameters - Greedy and ICON for full run | |
| registration_methods = ["Greedy"] # , "ICON", "Greedy_ICON"] | |
| number_of_iterations_list = [ | |
| [30, 15, 7, 3], | |
| ] # For ANTs | |
| ] # For Greedy | |
| # 20, # For ICON | |
| # [[30, 15, 7, 3], 20], # For ANTS_ICON | |
| # [[30, 15, 7, 3], 20], # For Greedy_ICON | |
| # ] | |
| # Registration parameters - Greedy for full run (ICON options commented out) | |
| registration_methods = ["Greedy"] # , "ICON", "Greedy_ICON"] | |
| number_of_iterations_list = [ | |
| [30, 15, 7, 3], | |
| ] # For Greedy | |
| # 20, # For ICON | |
| # [[30, 15, 7, 3], 20], # For Greedy_ICON | |
| # ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/Reconstruct4DCT/reconstruct_4d_ct_class.py` around lines 65 - 72,
Update the inline comment above the registration configuration to reflect the
active methods: change the comment "Registration parameters - Greedy and ICON
for full run" to indicate only Greedy is enabled (e.g., "Registration parameters
- Greedy for full run") and ensure the explanatory comment next to
number_of_iterations_list aligns (remove or adjust references to
ICON/Greedy_ICON) so the comments match the current registration_methods and
number_of_iterations_list variables.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
==========================================
+ Coverage 32.90% 32.92% +0.01%
==========================================
Files 53 53
Lines 7229 7229
==========================================
+ Hits 2379 2380 +1
+ Misses 4850 4849 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove the project guideline requiring axis order and shape to be stated in every docstring and comment touching arrays. ITK's (Z, Y, X) numpy convention is well-known and was adding noise without value. Comments for non-standard orderings (pynrrd, ANTs vector images) are kept.
Apply the same policy to source files: drop "Axis ordering:" subsections and inline (Z, Y, X) / (X, Y, Z) shape annotations from register_images_icon.py, image_tools.py, labelmap_tools.py, contour_tools.py, register_images_ants.py, register_images_greedy.py, convert_image_4d_to_3d.py, transform_tools.py, and test_tools.py.
Fix all callers that passed legacy method-name strings to RegisterTimeSeriesImages:
RegisterImagesANTS is unchanged; REGISTRATION_METHODS stays ["Greedy", "ICON", "Greedy_ICON"]. Baselines for affected slow tests will need regeneration.
Summary by CodeRabbit
Documentation
Refactor