Point wise implementation of frame-wise displacement #11
Conversation
WalkthroughAdds a new CLI tool that computes per-timepoint framewise displacement (mean and max) by extracting ROI points from a binary mask, reading a CSV of rigid-body transforms, applying transforms sequentially to points, and writing results to an output CSV. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant FS as File System
participant Mask as Mask Processor
participant Trans as Transform Reader
participant FD as FD Calculator
participant Out as Output Writer
User->>CLI: run mask_file, csv_file, [output_csv], --verbose
CLI->>FS: check files exist
FS-->>CLI: exist / error
CLI->>Mask: load mask, extract ROI voxel physical points
Mask->>FS: read mask file
Mask-->>CLI: ROI points
CLI->>Trans: read transforms CSV -> transforms[]
Trans->>FS: read CSV
Trans-->>CLI: transforms[]
CLI->>FD: initialize prev_points using transforms[0] (TransformPoint)
loop for each transform t in transforms[1:]
CLI->>FD: apply transform t -> cur_points
FD->>FD: compute Euclidean distances(cur_points, prev_points)
FD-->>CLI: mean_fd, max_fd for t
CLI->>FD: set prev_points = cur_points
end
CLI->>Out: write CSV (Timepoint, MeanFD, MaxFD)
Out->>FS: write output file
FS-->>Out: success
Out-->>CLI: done
CLI-->>User: print/return results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
framewise_displacement.py (2)
11-16: Remove unused helper function.This function is never called. The same logic is duplicated inline at lines 67-73. Consider removing this function to reduce code duplication and improve maintainability.
Apply this diff to remove the dead code:
-def calculate_mean_max_fd(points, transform, prev_points): - curr_points = np.array([transform.TransformPoint(p) for p in points]) - distances = np.linalg.norm(curr_points - prev_points, axis=1) - mean_fd = np.mean(distances) - max_fd = np.max(distances) - return mean_fd, max_fd -
31-31: Consider addingstrict=Trueto zip() for Python 3.10+ compatibility.As per static analysis hint, adding
strict=Trueensures all iterables have the same length. While not critical here (the arrays fromnp.whereare guaranteed to match), it's a defensive best practice.- for z, y, x in zip(z_idxs, y_idxs, x_idxs): + for z, y, x in zip(z_idxs, y_idxs, x_idxs, strict=True):
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
framewise_displacement.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framewise_displacement.py (1)
apply_transforms.py (1)
read_transforms_from_csv(10-56)
🪛 Ruff (0.14.8)
framewise_displacement.py
31-31: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🔇 Additional comments (1)
framewise_displacement.py (1)
1-9: LGTM!The imports are well-organized and appropriate for the tool's functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
framewise_displacement.py (2)
16-29: Validate that the mask is not empty.If the mask contains no non-zero voxels, the
pointslist will be empty. This will causenp.meanandnp.maxon lines 67-68 to returnnanor issue warnings. Add validation after extracting ROI points to fail fast with a clear error message.Apply this diff to add validation:
points = [] for z, y, x in zip(z_idxs, y_idxs, x_idxs): pt = mask.TransformIndexToPhysicalPoint((int(x), int(y), int(z))) points.append(pt) + if len(points) == 0: + print("Error: Mask contains no voxels.") + sys.exit(1) + if verbose: print(f"ROI contains {len(points)} voxels.")Based on past review comments, this issue was previously flagged but not yet addressed.
3-9: Movecsvimport to module level.The
csvmodule is imported locally on line 80 but should be imported at the top of the file with other standard library imports for consistency and clarity.Apply this diff to move the import:
+import csv import argparse import SimpleITK as sitkThen remove the local import on line 80:
if output_csv: with open(output_csv, 'w', newline='') as f: - import csv writer = csv.writer(f)Based on past review comments.
🧹 Nitpick comments (1)
framewise_displacement.py (1)
24-24: Consider addingstrict=Truetozip()(Python 3.10+).While the arrays from
np.whereare guaranteed to have the same length, addingstrict=Truemakes the intent explicit and follows Python 3.10+ best practices.Apply this diff:
- for z, y, x in zip(z_idxs, y_idxs, x_idxs): + for z, y, x in zip(z_idxs, y_idxs, x_idxs, strict=True):Note: This requires Python 3.10 or later. Only apply if the project's minimum Python version supports it.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
framewise_displacement.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framewise_displacement.py (1)
apply_transforms.py (1)
read_transforms_from_csv(10-56)
🪛 Ruff (0.14.8)
framewise_displacement.py
24-24: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🔇 Additional comments (3)
framewise_displacement.py (3)
44-54: LGTM: Correct initialization of reference points and baseline displacement.The initialization properly transforms ROI points using the first transform and correctly sets t=0 displacement to zero by definition. The comment about direct vs. inverse transforms is helpful context.
56-76: LGTM: Correct framewise displacement calculation.The loop correctly:
- Transforms points at each timepoint using the corresponding transform
- Computes Euclidean distances between consecutive point sets using
np.linalg.norm(axis=1)- Tracks mean and max displacement metrics
- Updates the reference points for the next iteration
The indexing is correct: loop variable
iranges from 0 tolen(transforms)-2, producing timepoints 1 tolen(transforms)-1.
92-111: LGTM: Well-structured CLI with proper validation.The CLI entry point correctly:
- Defines required and optional arguments with clear help text
- Validates file existence before processing
- Provides accurate description of the point-tracking approach
- Passes arguments to the calculation function
The description accurately reflects the implementation (addressed from past review comments).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
framewise_displacement.py (1)
3-9: Movecsvimport to module level.The
csvmodule is currently imported inside the function at line 84, which is non-idiomatic. Import it here with the other standard library imports.Apply this diff:
import argparse +import csv import SimpleITK as sitkThen remove the
import csvstatement from line 84:if output_csv: with open(output_csv, 'w', newline='') as f: - import csv writer = csv.writer(f)Based on past review comments.
🧹 Nitpick comments (1)
framewise_displacement.py (1)
24-24: Consider addingstrict=Truetozip()for consistency.While the arrays from
np.whereare guaranteed to have the same length, addingstrict=Truemakes the intent explicit and follows modern Python best practices.Apply this diff:
- for z, y, x in zip(z_idxs, y_idxs, x_idxs): + for z, y, x in zip(z_idxs, y_idxs, x_idxs, strict=True):Note: This requires Python 3.10+. If targeting earlier versions, skip this change.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
framewise_displacement.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framewise_displacement.py (1)
apply_transforms.py (1)
read_transforms_from_csv(10-56)
🪛 Ruff (0.14.8)
framewise_displacement.py
24-24: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🔇 Additional comments (6)
framewise_displacement.py (6)
11-18: LGTM!Function signature is well-designed with appropriate parameter names and defaults. The verbose logging for mask reading is helpful.
28-30: LGTM!The empty mask validation properly addresses the concern from past reviews, preventing downstream failures in
np.meanandnp.maxcalls.
35-41: LGTM!The validation correctly ensures at least 2 transforms are present before attempting framewise displacement calculation. The reuse of
read_transforms_from_csvpromotes code consistency across the codebase.
43-80: LGTM!The point-tracking framewise displacement implementation is correct:
- Properly initializes reference points from t=0 (line 51)
- Correctly sets t=0 displacement to zero (lines 53-58)
- Accurately computes per-timepoint displacements by comparing consecutive transformed point sets (lines 66-69)
- The comments about transform direction (lines 49, 65) are helpful and accurate
The use of
tqdmfor progress visualization is user-friendly.
82-92: LGTM!The conditional CSV output properly handles the optional
output_csvparameter (line 82), and the output format with clear headers is appropriate. This addresses the past concern about handlingoutput_csv=None.
96-112: LGTM!The CLI implementation is clean and includes proper input validation. The description "using point tracking" accurately reflects the implementation method, addressing the concern from past reviews.
|
I reviewed the script and it all looks sensible, no comments on my end. Based on my preliminary usage, the framewise displacement estimates seem sensible and similar to other methods. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.