MAINT: Add static type checking #28

jhlegarreta · 2024-12-20T17:24:48Z

Add static type checking: use mypy to ensure that variable and function calls are using the appropriate types.

Configure and run mypy checks in CI, including the spell checking.

Documentation:
https://mypy.readthedocs.io/en/stable/index.html

jhlegarreta · 2024-12-20T17:25:00Z

@effigies I see that in this repository the style checking is being done through an explicit call to ruff in a dedicated GHA workflow, as opposed to the way it is done in nireports which I watched as the inspiration for this PR:

nifreeze/.github/workflows/contrib.yml

Lines 22 to 25 in 7a48118

    
           - name: Lint NiFreeze 
        
             run: pipx run ruff check --diff 
        
           - name: Format NiFreeze 
        
             run: pipx run ruff format --diff

vs
https://github.com/nipreps/nireports/blob/main/tox.ini#L72
https://github.com/nipreps/nireports/blob/main/.github/workflows/build_test_deploy.yml#L159
https://github.com/nipreps/nireports/actions/runs/12416179089/job/34664200380

The latter seems to be more elegant and self-contained. Is there a canonical way to do this through the NiPreps?

codecov · 2024-12-20T19:34:27Z

Codecov Report

Attention: Patch coverage is 84.12698% with 10 lines in your changes missing coverage. Please review.

Project coverage is 68.10%. Comparing base (75d26c6) to head (02ff9c4).
Report is 30 commits behind head on main.

Files with missing lines	Patch %	Lines
src/nifreeze/data/pet.py	30.76%	9 Missing ⚠️
src/nifreeze/data/base.py	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   67.45%   68.10%   +0.65%     
==========================================
  Files          20       20              
  Lines         977      994      +17     
  Branches      129      130       +1     
==========================================
+ Hits          659      677      +18     
+ Misses        265      263       -2     
- Partials       53       54       +1

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

jhlegarreta · 2024-12-20T20:32:18Z

Looks like the ignore_missing_imports flag is not being considered:
https://github.com/nipreps/nifreeze/pull/28/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R141
e.g.
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936?pr=28#step:8:48

I would have thought that mypy would consider it somehow when executing:
https://github.com/nipreps/nifreeze/pull/28/files#diff-ef2cef9f88b4fe09ca3082140e67f5ad34fb65fb6e228f119d3812261ae51449R56

Any clue @effigies ?

jhlegarreta · 2024-12-22T02:02:11Z

Spent a few hours on this; solved all those warnings that I could: down to 10 8 locally using the --ignore-missing-import flag and including changes in PRs #29, #30 and #44. A few notes:

The error
```
src/nifreeze/model/gpr.py:187: error:
Unexpected keyword argument "n_targets" for "__init__" of "GaussianProcessRegressor"  [call-arg]
```
seems legitimate: we are requiring scikit-learn>=0.18 (a pretty old one) and n_targets did not appear until scikit_learn==1.3.0. However, testing is using scikit-learn 1.6.0:
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:29
which does have it https://scikit-learn.org/1.6/modules/generated/sklearn.gaussian_process.GaussianProcessRegressor.html
So not sure what's going on here.
I'd dare to say that the NiBabel load function seems to have some misleading type hints:
https://github.com/nipy/nibabel/blob/59733ff5a9eedb73e63261a1d706bfe1efbf0f50/nibabel/loadsave.py#L82
which results in mypy complaining, e.g.
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:63:
```
scripts/dwi_gp_estimation_signal_plot.py:113: error:
"FileBasedImage" has no attribute "get_fdata"  [attr-defined]
```
https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:142:
```
scripts/optimize_registration.py:173: error:
Argument 1 to "displacements_within_mask" has incompatible type "FileBasedImage";
expected "SpatialImage"  [arg-type]
```
Have tried to pin this down in NiBabel without success. Am I missing something @effigies? Can push a patch to NiBabel if you give me some context/doable for a non-expert.

Edit: OK, I've seen what was done in type: Annotate tools and reportlets.utils nireports#147 and have replicated it in PR ENH: Fix nibabel image type checking errors #44 (made a separate PR for easier handling/as it adds new code). I guess this is a temporary solution until issues are fixed in NiBabel.
Preferred to do separate PRs for these as they involve more than just type hints: DOC: Revision of docstrings in the iterators' module #29, ENH: Fix GP estimation error analysis script #30, ENH: Fix nibabel image type checking errors #44

README.rst

docs/conf.py

src/nifreeze/cli/parser.py

src/nifreeze/model/gpr.py

src/nifreeze/testing/simulations.py

oesteban · 2025-01-15T10:22:19Z

seems legitimate: we are requiring scikit-learn>=0.18 (a pretty old one) and n_targets did not appear until scikit_learn==1.3.0. However, testing is using scikit-learn 1.6.0:

We should then upgrade the pinning.

jhlegarreta · 2025-01-15T14:26:30Z

We should then upgrade the pinning.

Do you want that to be done in this PR or in a separate one so as to avoid the change going unnoticed among all these commits? The potential new PR could be merged before this one, then this one rebased.

effigies

mypy is now passing on this project. I think the changes I made were in the spirit of what is intended here.

effigies · 2025-01-22T19:53:02Z

src/nifreeze/data/base.py

@@ -52,7 +58,7 @@ def _cmp(lh: Any, rh: Any) -> bool:


 @attr.s(slots=True)
-class BaseDataset:
+class BaseDataset(Generic[*Ts]):


This is the main new thing I introduced. This BaseDataset class needs to permit a dynamic number of return values for __getitem__, dependent on the subclass. In the future, we should be able to set this as a default:

class BaseDataset[*Ts=Unpack[tuple[()]]]: ...

Which, while not particularly pretty, will allow us not to have to type BaseDataset[()] when annotating a variable that is actually this superclass.

effigies

I've reviewed the whole thing, including my own contributions, and I'm good with this. It could probably do with another pair of eyes.

Group keyword arguments into a single dictionary. Fixes: ``` scripts/optimize_registration.py:133: error: Argument "output_transform_prefix" to "generate_command" has incompatible type "str"; expected "dict[Any, Any]" [arg-type] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:141

Use `str` instead of `Path` for `_parse_yaml_config` parameter since `argparse` passes the argument as a string, not as a `Path`. Fixes: ``` src/nifreeze/cli/parser.py:74: error: Argument "type" to "add_argument" of "_ActionsContainer" has incompatible type "Callable[[Path], dict[Any, Any]]"; expected "Callable[[str], Any] | FileType | str" [arg-type] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:47

List `sigma_sq` in the GP model slots. Fixes: ``` src/nifreeze/model/_dipy.py:128: error: Trying to assign name "sigma_sq" that is not in "__slots__" of type "nifreeze.model._dipy.GaussianProcessModel" [misc] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:102

Add the dimensionality to the `mask` ndarray parameter annotation. Fixes: ``` src/nifreeze/model/_dipy.py:140: error: "ndarray" expects 2 type arguments, but 1 given [type-arg] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:103

Fix type hint for `figsize` parameter: use `float` as values can be floating point numbers. Fixes: ``` src/nifreeze/viz/signals.py:40: error: Incompatible default for argument "figsize" (default has type "tuple[float, float]", argument has type "tuple[int, int]") [assignment] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:49

Provide appropriate type hints to the `reg_target_type` parameter of the `ants._run_registration` function. The expression is set to a tuple in https://github.com/nipreps/nifreeze/blob/6b6ba707c47b2763c040f3d3a2ceabdcf450eefe/src/nifreeze/estimator.py#L103 Fixes: ``` src/nifreeze/registration/ants.py:459: error: Incompatible types in assignment (expression has type "tuple[str, str]", variable has type "str") [assignment] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:133

Import `Bounds` from `scipy.optimize`. Fixes: ``` src/nifreeze/model/gpr.py:32: error: Module "scipy.optimize._minimize" does not explicitly export attribute "Bounds" [attr-defined] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:70

Avoid type checking for private function import statement. Fixes: ``` src/nifreeze/model/gpr.py:215: error: Module "sklearn.utils.optimize" has no attribute "_check_optimize_result" [attr-defined] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:73

Remove unused `namedtuple` definition in test. Fixes: ``` test/test_gpr.py:31: error: First argument to namedtuple() should be "GradientTablePatch", not "gtab" [name-match] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:129

Use `ClassVar` for type hinting the `GaussianProcessRegressor` `_parameter_constraints` class variable in child class. Fixes: ``` src/nifreeze/model/gpr.py:156: error: Cannot override class variable (previously declared on base class "GaussianProcessRegressor") with instance variable [misc] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:71

Annotate the `optimizer` attribute type in the DiffusionGPR GPR child class. Fixes: ``` src/nifreeze/model/gpr.py:234: error: "DiffusionGPR" has no attribute "optimizer" [attr-defined] ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:82

Requires making BaseDataset generic on indexable fields

model.predict can return 1, 2 or 3 arrays

Annotate `Kernel.diag()` argument `X`: use `npt.ArrayLike`. Fixes: ``` src/nifreeze/model/gpr.py:335: error: Argument 1 of "diag" is incompatible with supertype "Kernel"; supertype defines the argument type as "Buffer | _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | bool | int | float | complex | str | bytes | _NestedSequence[bool | int | float | complex | str | bytes]" [override] src/nifreeze/model/gpr.py:335: note: This violates the Liskov substitution principle src/nifreeze/model/gpr.py:335: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides src/nifreeze/model/gpr.py:445: error: Argument 1 of "diag" is incompatible with supertype "Kernel"; supertype defines the argument type as "Buffer | _SupportsArray[dtype[Any]] | _NestedSequence[_SupportsArray[dtype[Any]]] | bool | int | float | complex | str | bytes | _NestedSequence[bool | int | float | complex | str | bytes]" [override] src/nifreeze/model/gpr.py:445: note: This violates the Liskov substitution principle src/nifreeze/model/gpr.py:335: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides ``` raised for example in: https://github.com/nipreps/nifreeze/actions/runs/12437972140/job/34728973936#step:8:93 Documentation: https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

oesteban · 2025-01-23T08:01:24Z

This is such a big PR I fear we will not be able to ever finish it if we stop at every rock on the way. Let's merge this one in and then resolve side effects and nuances (#29, #30, #64) later.

jhlegarreta force-pushed the AddStaticTypeChecking branch from 8baa17c to fdf477e Compare December 20, 2024 19:25

jhlegarreta force-pushed the AddStaticTypeChecking branch 2 times, most recently from 9e3f60a to e050924 Compare December 20, 2024 20:23

jhlegarreta force-pushed the AddStaticTypeChecking branch 4 times, most recently from 363c6e8 to 470c510 Compare December 22, 2024 00:29

jhlegarreta mentioned this pull request Dec 22, 2024

BUG: Fix SphericalKriging instantiation parameter names in scripts #41

Closed

oesteban reviewed Jan 15, 2025

View reviewed changes

README.rst Outdated Show resolved Hide resolved

docs/conf.py Outdated Show resolved Hide resolved

src/nifreeze/cli/parser.py Show resolved Hide resolved

src/nifreeze/model/gpr.py Show resolved Hide resolved

src/nifreeze/testing/simulations.py Outdated Show resolved Hide resolved

jhlegarreta force-pushed the AddStaticTypeChecking branch from 470c510 to e518c42 Compare January 15, 2025 14:00

jhlegarreta force-pushed the AddStaticTypeChecking branch 2 times, most recently from 39bbf76 to 516d0d7 Compare January 15, 2025 19:36

jhlegarreta mentioned this pull request Jan 18, 2025

ENH: Refactor data representation structures for multimodality #52

Merged

oesteban added a commit that referenced this pull request Jan 20, 2025

enh: minor edits to minimize conflicts with #28.

68ccc85

jhlegarreta mentioned this pull request Jan 20, 2025

MAINT: Update minimum required scikit-learn version to 1.3.0 #58

Merged

oesteban changed the title ~~ENH: Add static type checking~~ MAINT: Add static type checking Jan 20, 2025

jhlegarreta pushed a commit to jhlegarreta/nifreeze that referenced this pull request Jan 20, 2025

enh: minor edits to minimize conflicts with nipreps#28.

09b391c

oesteban mentioned this pull request Jan 20, 2025

Decision on adopting type hints #24

Closed

jhlegarreta force-pushed the AddStaticTypeChecking branch from d5268d7 to 88d2565 Compare January 20, 2025 16:58

effigies self-assigned this Jan 21, 2025

effigies reviewed Jan 22, 2025

View reviewed changes

effigies approved these changes Jan 22, 2025

View reviewed changes

jhlegarreta mentioned this pull request Jan 23, 2025

sty: Address ruff complaints #64

Merged

jhlegarreta force-pushed the AddStaticTypeChecking branch from bd38772 to c5ac831 Compare January 23, 2025 02:44

jhlegarreta and others added 16 commits January 22, 2025 22:37

chore: Ignore more untyped dependencies

052cf36

type: Fix annotations in data.base and utils.iterators

374fe48

type: Fix expected type of set_xticklabels arg

478c860

chore: Fix DiffusionGPR annotations

0037efd

jhlegarreta force-pushed the AddStaticTypeChecking branch from e03f7f2 to 126a5bb Compare January 23, 2025 03:37

effigies added 7 commits January 22, 2025 22:40

type: Annotate PET Dataset

9f3fdd5

Requires making BaseDataset generic on indexable fields

type: Fix annotations for dMRI model

05237a6

type: Fix complaint about mismatched return type

4eb80dd

model.predict can return 1, 2 or 3 arrays

type: Ignore warning related to bad upstream stubs

e240ac6

type: Fixes

b450b8d

doc: Unmock numpy to allow np.ndarray to be found as a type

2310945

jhlegarreta force-pushed the AddStaticTypeChecking branch from 126a5bb to 02ff9c4 Compare January 23, 2025 03:40

oesteban merged commit d9b390b into nipreps:main Jan 23, 2025
11 checks passed

jhlegarreta deleted the AddStaticTypeChecking branch January 23, 2025 13:55

This was referenced Jan 23, 2025

ENH: Fix GP estimation error analysis script #30

Draft

Direct edit review: Fixing test.pypi.org upload #81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: Add static type checking #28

MAINT: Add static type checking #28

jhlegarreta commented Dec 20, 2024

jhlegarreta commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

jhlegarreta commented Dec 20, 2024

jhlegarreta commented Dec 22, 2024 •

edited

Loading

oesteban commented Jan 15, 2025

jhlegarreta commented Jan 15, 2025

effigies left a comment

effigies Jan 22, 2025

effigies left a comment

oesteban commented Jan 23, 2025

MAINT: Add static type checking #28

MAINT: Add static type checking #28

Conversation

jhlegarreta commented Dec 20, 2024

jhlegarreta commented Dec 20, 2024 • edited Loading

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

jhlegarreta commented Dec 20, 2024

jhlegarreta commented Dec 22, 2024 • edited Loading

oesteban commented Jan 15, 2025

jhlegarreta commented Jan 15, 2025

effigies left a comment

Choose a reason for hiding this comment

effigies Jan 22, 2025

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

oesteban commented Jan 23, 2025

jhlegarreta commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

jhlegarreta commented Dec 22, 2024 •

edited

Loading