Skip to content

Conversation

@jhlegarreta
Copy link
Contributor

Add type hints to PET-related classes and functions.

@jhlegarreta jhlegarreta force-pushed the sty/add-pet-type-hints branch 6 times, most recently from d6ba066 to a1e9842 Compare July 26, 2025 18:50
@jhlegarreta jhlegarreta requested a review from mnoergaard July 26, 2025 20:20
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Jul 26, 2025

I left data.PET.to_nifti out as when adding type hinting for the existing parameters mypy complained in important ways about it deviating from the base class signature. So, it needs refactoring. Outside the scope of this PR.

@jhlegarreta jhlegarreta force-pushed the sty/add-pet-type-hints branch 2 times, most recently from 119ec4e to 20eaceb Compare August 18, 2025 22:21
@jhlegarreta
Copy link
Contributor Author

Failures:

src/nifreeze/model/pet.py:163: error: Argument 1 to "design_matrix" of "BSpline" has incompatible type "float"; expected "_CanArrayND[floating[Any] | integer[Any] | bool_] | Sequence[float | floating[Any] | integer[Any] | bool_]"  [arg-type]

and

src/nifreeze/estimator.py:269: error: Argument 1 to "fit_predict" of "PETModel" has incompatible type "ndarray[Any, Any] | None"; expected "int | None"  [arg-type]

So, the second one is related to the fact that fit_predict expects an int for the index but lofo_split returns an array for test_time.

@codecov
Copy link

codecov bot commented Aug 18, 2025

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.71%. Comparing base (4cfaadb) to head (ff4a9fa).

Files with missing lines Patch % Lines
src/nifreeze/data/pet.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   75.64%   75.71%   +0.06%     
==========================================
  Files          24       24              
  Lines        1433     1437       +4     
  Branches      166      166              
==========================================
+ Hits         1084     1088       +4     
  Misses        276      276              
  Partials       73       73              

☔ 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 sty/add-pet-type-hints branch from 20eaceb to 635fe57 Compare August 18, 2025 22:54
@jhlegarreta
Copy link
Contributor Author

I am leaving out lofo_split and other problematic ones for the sake of moving forward. lofo_split is anyways likely to be removed by PR #203. So adding type hints to those/issues will be addressed in future PRs.

@jhlegarreta jhlegarreta force-pushed the sty/add-pet-type-hints branch from e144476 to 0900bff Compare September 26, 2025 01:28
Add type hints to PET-related classes and functions.

Notably, use positional arguments instead of keyword arguments when
instantiating a `namedtuple`.
@jhlegarreta jhlegarreta force-pushed the sty/add-pet-type-hints branch from 0900bff to ff4a9fa Compare September 26, 2025 01:37
@jhlegarreta
Copy link
Contributor Author

Merging. Checked that the output of the notebook is the same that we get prior to the change (modulo slight differences at the beginning/end of the sequence: x-ref #204 (comment)):

pet_volumewise_motion_estimated pet_volumewise_motion_ground_truth pet_framewise_displacement

Wanted to make sure due to these changes:
https://github.com/nipreps/nifreeze/pull/189/files#diff-cf38fd0d1c160d335eaf2315eb3076ecbb8e99d387b5f642571190c033a0e5beR122
https://github.com/nipreps/nifreeze/pull/189/files#diff-cf38fd0d1c160d335eaf2315eb3076ecbb8e99d387b5f642571190c033a0e5beR164

@jhlegarreta jhlegarreta merged commit f6199eb into nipreps:main Sep 26, 2025
9 of 10 checks passed
@jhlegarreta jhlegarreta deleted the sty/add-pet-type-hints branch September 26, 2025 23:10
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.

1 participant