Skip to content

Conversation

@jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Aug 9, 2025

Refactor PET estimator so that it is more closely aligned with the Estimator class, and in order to eventually merge them.

@jhlegarreta jhlegarreta force-pushed the ref/refactor-pet-estimator branch from 8c7d13a to f0862e6 Compare August 13, 2025 23:05
@jhlegarreta
Copy link
Contributor Author

I think this is close enough to the general estimator class.

I compared the current version and the one in the patch set:

  • The one in the patch set is 6x faster (5:26 vs 32:10).
  • Results are different, though.

Version the PETEstimator in main:

pet_estimated_motion_main pet_fd_main

Using the PETEstimator in this patch set:

pet_estimated_motion_ps pet_fd_ps

And none seems to be doing a good job: I have plot the ground truth:

pet_sub02_groundtruth

Given that none is doing a good job, and unless you clearly see the fix for the original PETEstimator @mnoergaard, my opinion is that this should be merged so that we invest our time better fixing a version that is closer to the Estimator class so that merging both is eventually easier.

@jhlegarreta jhlegarreta force-pushed the ref/refactor-pet-estimator branch 5 times, most recently from 0813f9a to c112c87 Compare August 21, 2025 23:46
@codecov
Copy link

codecov bot commented Aug 22, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.55%. Comparing base (4a96c3c) to head (c49905a).
⚠️ Report is 80 commits behind head on main.

Files with missing lines Patch % Lines
src/nifreeze/estimator.py 76.92% 7 Missing and 2 partials ⚠️
src/nifreeze/model/base.py 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
- Coverage   75.95%   75.55%   -0.40%     
==========================================
  Files          24       24              
  Lines        1439     1432       -7     
  Branches      166      165       -1     
==========================================
- Hits         1093     1082      -11     
- Misses        275      280       +5     
+ Partials       71       70       -1     

☔ 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/refactor-pet-estimator branch 4 times, most recently from 2848477 to c49905a Compare September 28, 2025 01:21
@jhlegarreta jhlegarreta force-pushed the ref/refactor-pet-estimator branch 3 times, most recently from 1bdb894 to 8b955c4 Compare October 28, 2025 10:34
Refactor PET estimator so that it is more closely aligned with the
`Estimator` class, and in order to eventually merge them.

Most notably, avoid creating train, test splits inside the estimator and
delegate the task of walking the sequence to the existing iterators.
This removes the need for the `lofo_split` method.
@jhlegarreta jhlegarreta force-pushed the ref/refactor-pet-estimator branch from 8b955c4 to 8aac2fd Compare October 28, 2025 10:39
pbar.set_description_str(f"{FIT_MSG: <16} vol. <{i}>")

# Fit the model once on the training dataset
model.fit_predict(None)
Copy link
Member

Choose a reason for hiding this comment

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

@mnoergaard is this really what we want?

It can be done, but calling fit_predict(None) is meant to be done outside the iteration loop (i.e., fit only once in all data and after that fit_predict is only "predict").

Is there a reason to fit everytime (btw, in the case of diffusion models, if fit once with all data then the object is locked and doesn't fit again).

Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss this tomorrow @jhlegarreta

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