Skip to content

REF: Remove PET estimator#426

Merged
jhlegarreta merged 1 commit into
nipreps:mainfrom
jhlegarreta:ref/remove-pet-estimator
Mar 22, 2026
Merged

REF: Remove PET estimator#426
jhlegarreta merged 1 commit into
nipreps:mainfrom
jhlegarreta:ref/remove-pet-estimator

Conversation

@jhlegarreta
Copy link
Copy Markdown
Contributor

@jhlegarreta jhlegarreta commented Feb 14, 2026

Remove the PET estimator: the NiFreeze estimator design is modality-agnostic, so a PET-specific estimator is not required.

Remove the PET-specific integration test: checking that motion estimation with PET data yields the identity matrix when monkey patching the transformation computation when the estimator runs the registration step is covered by the
test_estimator:test_estimator_iterator_index_match test function.

Update the notebook accordingly:

  • Avoid trying to fit on all data, as it is not implemented yet
    following commit a468d76.
  • Call .matrix on the Affine instances to get the affine transformation as a NumPy array.

@jhlegarreta
Copy link
Copy Markdown
Contributor Author

Supersedes PR #203.

Depends on PR #424.

@jhlegarreta jhlegarreta force-pushed the ref/remove-pet-estimator branch from 1c7e6f7 to 3049c10 Compare February 14, 2026 15:28
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.37%. Comparing base (db0286b) to head (7007918).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #426      +/-   ##
==========================================
- Coverage   83.91%   83.37%   -0.54%     
==========================================
  Files          37       37              
  Lines        2138     2093      -45     
  Branches      235      231       -4     
==========================================
- Hits         1794     1745      -49     
- Misses        303      309       +6     
+ Partials       41       39       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhlegarreta jhlegarreta force-pushed the ref/remove-pet-estimator branch 2 times, most recently from 4d97eef to 5124bc8 Compare March 21, 2026 23:17
Remove the PET estimator: the `NiFreeze` estimator design is
modality-agnostic, so a PET-specific estimator is not required.

Remove the PET-specific integration test: checking that motion
estimation with PET data yields the identity matrix when monkey patching
the transformation computation when the estimator runs the registration
step is covered by the
`test_estimator:test_estimator_iterator_index_match` test function.

Update the notebook accordingly:
- Avoid trying to fit on all data, as it is not implemented yet
  following commit a468d76.
- Call `.matrix` on the `Affine` instances to get the affine
  transformation as a `NumPy` array.
@jhlegarreta jhlegarreta force-pushed the ref/remove-pet-estimator branch from 5124bc8 to 7007918 Compare March 21, 2026 23:29
@jhlegarreta
Copy link
Copy Markdown
Contributor Author

@oesteban @mnoergaard Runs from beginning to end now. The estimation is still not good, but that is independent of the patch set I'd say.

@jhlegarreta jhlegarreta marked this pull request as ready for review March 21, 2026 23:41
Copy link
Copy Markdown
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

This is great. We can now work on making the general estimator work properly, but having a net decrease of ~2400 lines is a substantial complexity reduction. Thanks!

@jhlegarreta jhlegarreta merged commit bf22233 into nipreps:main Mar 22, 2026
9 of 10 checks passed
@jhlegarreta jhlegarreta deleted the ref/remove-pet-estimator branch March 22, 2026 14:02
@jhlegarreta
Copy link
Copy Markdown
Contributor Author

One thing that came to my mind yesterday was nipy/nitransforms#286 (comment). Maybe we are not extracting the motion parameters correctly, and hence, our results do not seem to be correct. May not be the only reason.

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.

2 participants