REF: Refactor PET estimator

[jhlegarreta]

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

[jhlegarreta]

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:

Note that the increase in error towards the end of the sequence is expected as the signal available in the final frames is low, and thus, estimates are expected to be less good.

Using the PETEstimator in this patch set:

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

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.

[codecov]

Codecov Report

❌ Patch coverage is 76.92308% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.24%. Comparing base (e1b70cd) to head (6a1f5af).

Files with missing lines	Patch %	Lines
src/nifreeze/estimator.py	76.92%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
- Coverage   82.29%   82.24%   -0.06%     
==========================================
  Files          37       37              
  Lines        2056     2044      -12     
  Branches      227      226       -1     
==========================================
- Hits         1692     1681      -11     
+ Misses        319      318       -1     
  Partials       45       45              

☔ 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.

[oesteban]

@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).

[oesteban]

Let's discuss this tomorrow @jhlegarreta

[jhlegarreta]

Related to my question in #197 (comment). Removing that from the estimator allows the notebook to run and generate the results in #203 (comment), but removing it from the notebook without removing the immediate cell on predicting the given index results in an error: https://github.com/jhlegarreta/nifreeze/blob/9b36f58075eeea6b8234274f0ecdfe0d015f94c2/src/nifreeze/model/pet.py#L184. Removing the 2 calls to fit_predict(None) (notebook and the pointed location in the estimator), as well as the explicit call to fit_predict in the notebook, allows the notebook to run. Results are still the same as in #203 (comment). So I would suggest that @mnoergaard has a look at the related issues.

[jhlegarreta]

The execution of the notebook with the current patch set gives the following results:

Ground truth motion

Estimated motion (the range is different, but differences are notable wrt the ground truht)

FD

So, not working. But such was the case previously as well (#203 (comment)). So the underlying issues is probably elsewhere, whether in the model or the test data, rather than the estimator.

I think that this is ready to be merged; the idea is to bring the PETMotionEstimator class closer to the Estimator class to eventually remove the former.