Skip to content

Chore(Next-Gen): enable pre-commits for CAREamistV2#833

Merged
jdeschamps merged 6 commits intomainfrom
jd/chore/careamist_v2_pre-commit
Mar 27, 2026
Merged

Chore(Next-Gen): enable pre-commits for CAREamistV2#833
jdeschamps merged 6 commits intomainfrom
jd/chore/careamist_v2_pre-commit

Conversation

@jdeschamps
Copy link
Copy Markdown
Member

Description

Following #827, this PR enables pre-commit hooks for CAREamistV2 and attempts to solve the errors.

Note that there is something fishy with the use of TypeVar that mypy disagrees with:

src/careamics/careamist_v2.py:128: error: Argument 1 to "_load_model" of "CAREamistV2" has incompatible type "NGConfiguration[CAREAlgorithm] | Path | None"; expected "NGConfiguration[AlgorithmConfig] | Path | None"  [arg-type]
src/careamics/careamist_v2.py:128: error: Argument 1 to "_load_model" of "CAREamistV2" has incompatible type "NGConfiguration[N2NAlgorithm] | Path | None"; expected "NGConfiguration[AlgorithmConfig] | Path | None"  [arg-type]
src/careamics/careamist_v2.py:128: error: Argument 1 to "_load_model" of "CAREamistV2" has incompatible type "NGConfiguration[N2VAlgorithm] | Path | None"; expected "NGConfiguration[AlgorithmConfig] | Path | None"  [arg-type]
Found 3 errors in 1 file (checked 1 source file)

One way to solve it to redefine AlgorithmConfig in the same module, rather than importing it. It seems that allows "bounding" the type to the functions it is used in.

But it would be annoying to have to redefine the generics in every module. I decided to just ignore it for now for two reasons:

@jdeschamps jdeschamps changed the title Jd/chore/careamist v2 pre commit Chore(Next-Gen): enable pre-commits for CAREamistV2 Mar 20, 2026
@jdeschamps jdeschamps requested a review from a team March 20, 2026 14:30
@melisande-c
Copy link
Copy Markdown
Member

hmm yeah I think maybe this is one of those things pyright is fine with but mypy complains, and now I don't remember why I made the configuration a generic, sorry for complicating things. But a solution here is to replace the typing of the config arg in the __init__ to this:

NGConfigurationType = (
    NGConfiguration[CAREAlgorithm]
    | NGConfiguration[N2NAlgorithm]
    | NGConfiguration[N2VAlgorithm]
)

@jdeschamps
Copy link
Copy Markdown
Member Author

jdeschamps commented Mar 23, 2026

hmm yeah I think maybe this is one of those things pyright is fine with but mypy complains, and now I don't remember why I made the configuration a generic, sorry for complicating things. But a solution here is to replace the typing of the config arg in the __init__ to this:

NGConfigurationType = (
    NGConfiguration[CAREAlgorithm]
    | NGConfiguration[N2NAlgorithm]
    | NGConfiguration[N2VAlgorithm]
)

No need to apologize. It is a bit annoying that generics are so annoying in Python (at least for mypy). Maybe another nail in the coffin.

Implemented the change in 68b39be

Copy link
Copy Markdown
Member

@melisande-c melisande-c left a comment

Choose a reason for hiding this comment

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

Looks good

val_data_target=val_data_target,
train_data_mask=filtering_mask,
loading=loading,
loading=loading, # type: ignore
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

damn it something else that works with pyright but not mypy, this is happening bec mypy can't resolve overloads that well

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep... Moving to pyright is just going to feel good I surmise.

@jdeschamps jdeschamps merged commit 9e911f2 into main Mar 27, 2026
12 of 13 checks passed
@jdeschamps jdeschamps deleted the jd/chore/careamist_v2_pre-commit branch March 27, 2026 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants