-
-
Notifications
You must be signed in to change notification settings - Fork 425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Maint: Bump mypy #5727
Maint: Bump mypy #5727
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5727 +/- ##
==========================================
+ Coverage 89.83% 89.86% +0.02%
==========================================
Files 608 608
Lines 51856 51893 +37
==========================================
+ Hits 46587 46632 +45
+ Misses 5269 5261 -8
... and 12 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Czaki I'm not a fan of some of the changes in this PR. I'm not super familiar with where typing is going, so I'm open to discussion, but by and large I really dislike weird type aliases/custom types for simple things like dicts. It makes the code much harder to read for a newcomer. An _ExtraDict
is not a thing anyone knows about or will ever know about because it's a completely niche item. But a dictionary is immediately understandable to even Python beginners. So personally I would prefer to lose some specificity in the typing annotations and keep the data types as simple dictionaries.
ArrayBase = np.ndarray | ||
ArrayBase: Type[np.ndarray] = np.ndarray | ||
|
||
|
||
ImageData = NewType("ImageData", ArrayBase) | ||
LabelsData = NewType("LabelsData", ArrayBase) | ||
PointsData = NewType("PointsData", ArrayBase) | ||
ShapesData = NewType("ShapesData", List[ArrayBase]) | ||
SurfaceData = NewType("SurfaceData", Tuple[ArrayBase, ArrayBase, ArrayBase]) | ||
TracksData = NewType("TracksData", ArrayBase) | ||
VectorsData = NewType("VectorsData", ArrayBase) | ||
ImageData = NewType("ImageData", np.ndarray) | ||
LabelsData = NewType("LabelsData", np.ndarray) | ||
PointsData = NewType("PointsData", np.ndarray) | ||
ShapesData = NewType("ShapesData", List[np.ndarray]) | ||
SurfaceData = NewType("SurfaceData", Tuple[np.ndarray, np.ndarray, np.ndarray]) | ||
TracksData = NewType("TracksData", np.ndarray) | ||
VectorsData = NewType("VectorsData", np.ndarray) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these changed? It's incorrect to say that these data are NumPy arrays — we support many other types of arrays. The idea was to expand ArrayBase to those arrays in the future, rather than to hardcode np.ndarray.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is mypy error
napari/types.py:82: error: Argument 2 to NewType(...) must be subclassable (got ArrayBase?) [valid-newtype]
napari/types.py:82: error: Variable "napari.types.ArrayBase" is not valid as a type [valid-type]
napari/types.py:82: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
NewType
cannot be used for described scenario. From documentation
In contrast, NewType declares one type to be a subtype of another
Did we would like to convert it to ArrayLike
I could try to rewrite it, but I'm unsure which specification to specify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we would like to convert it to ArrayLike
I think I like this solution better than the current solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here error for ArrayLike
napari/types.py:82: error: Argument 2 to NewType(...) must be subclassable (got "Union[_SupportsArray[dtype[Any]], _NestedSequence[_SupportsArray[dtype[Any]]], bool, int, float, complex, str, bytes, _NestedSequence[Union[bool, int, float, complex, str, bytes]]]") [valid-newtype]
It looks like NewType requires contract class. We may try to use TypeVar
What did you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm out of my depth with typing but TypeVar could be good option...? I was never clear of the advantages/disadvantages between the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new type is way to declare a dummy subclass of same class to distinguish them went type checking.
Like
Kilogram = NewType("Kilogram", float)
Pound = NewType("Pound", float)
p = Pound(3)
def some_fun(a: Kilogram):
pass
some_fun(p) # mypy have error here
Where TypeVar
is a little more powerful type alias. When in above situation mypy will not point a problem. But You still have more meaningful names.
If we would like to have option to validate usage using mypy then most probably for we will end with something like:
ImageData_np = NewType("ImageData", np.ndarray)
ImageData_da = NewType("ImageData", da.Array)
...
ImageData = Union[ImageData_np, ImageData_da, ...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that last option tbh!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I convert to this approach Image layer. At this moment, I think that for all other layers, we require np.ndarray
.
there is some problems with mypy and zarr that I do not have time to solve at this moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. The problem is the lack of annotation in zarr package...
In this case mypy is to agresive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of union requires pyapp-kit/magicgui#546
+1 to @jni's comments, were any of these changes necessary when bumping mypy? |
@alisterburt All these changes were enforced by upgrading mypy. I touch only lines that mypy marks as wrong. I have updated a little code and have a few questions. |
This reverts commit c6d22e7.
<!-- In general, PRs should fix an existing issue on the repo. --> <!-- Please link to that issue here as "Closes #(issue-number)". --> Closes #5724 <!-- What does this pull request (PR) do? Why is it necessary? --> <!-- Tell us about your new feature, improvement, or fix! --> <!-- If your change includes user interface changes, please add an image, or an animation "An image is worth a thousand words!" --> <!-- You can use https://www.cockos.com/licecap/ or similar to create animations --> <!-- What resources, documentation, and guides were used in the creation of this PR? --> <!-- Please delete options that are not relevant. --> - [x] Bug-fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update <!-- Please describe the tests that you ran to verify your changes. --> - [ ] example: the test suite for my feature covers cases x, y, and z - [ ] example: all tests pass with my change - [ ] example: I check if my changes works with both PySide and PyQt backends as there are small differences between the two Qt bindings. - [ ] My PR is the minimum possible work for the desired functionality - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] If I included new strings, I have used `trans.` to make them localizable. For more information see our [translations guide](https://napari.org/developers/translations.html).
<!-- In general, PRs should fix an existing issue on the repo. --> <!-- Please link to that issue here as "Closes #(issue-number)". --> Closes #5724 <!-- What does this pull request (PR) do? Why is it necessary? --> <!-- Tell us about your new feature, improvement, or fix! --> <!-- If your change includes user interface changes, please add an image, or an animation "An image is worth a thousand words!" --> <!-- You can use https://www.cockos.com/licecap/ or similar to create animations --> <!-- What resources, documentation, and guides were used in the creation of this PR? --> <!-- Please delete options that are not relevant. --> - [x] Bug-fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update <!-- Please describe the tests that you ran to verify your changes. --> - [ ] example: the test suite for my feature covers cases x, y, and z - [ ] example: all tests pass with my change - [ ] example: I check if my changes works with both PySide and PyQt backends as there are small differences between the two Qt bindings. - [ ] My PR is the minimum possible work for the desired functionality - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] If I included new strings, I have used `trans.` to make them localizable. For more information see our [translations guide](https://napari.org/developers/translations.html).
<!-- In general, PRs should fix an existing issue on the repo. --> <!-- Please link to that issue here as "Closes #(issue-number)". --> Closes #5724 <!-- What does this pull request (PR) do? Why is it necessary? --> <!-- Tell us about your new feature, improvement, or fix! --> <!-- If your change includes user interface changes, please add an image, or an animation "An image is worth a thousand words!" --> <!-- You can use https://www.cockos.com/licecap/ or similar to create animations --> <!-- What resources, documentation, and guides were used in the creation of this PR? --> <!-- Please delete options that are not relevant. --> - [x] Bug-fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update <!-- Please describe the tests that you ran to verify your changes. --> - [ ] example: the test suite for my feature covers cases x, y, and z - [ ] example: all tests pass with my change - [ ] example: I check if my changes works with both PySide and PyQt backends as there are small differences between the two Qt bindings. - [ ] My PR is the minimum possible work for the desired functionality - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] If I included new strings, I have used `trans.` to make them localizable. For more information see our [translations guide](https://napari.org/developers/translations.html).
<!-- In general, PRs should fix an existing issue on the repo. --> <!-- Please link to that issue here as "Closes #(issue-number)". --> Closes #5724 <!-- What does this pull request (PR) do? Why is it necessary? --> <!-- Tell us about your new feature, improvement, or fix! --> <!-- If your change includes user interface changes, please add an image, or an animation "An image is worth a thousand words!" --> <!-- You can use https://www.cockos.com/licecap/ or similar to create animations --> <!-- What resources, documentation, and guides were used in the creation of this PR? --> <!-- Please delete options that are not relevant. --> - [x] Bug-fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update <!-- Please describe the tests that you ran to verify your changes. --> - [ ] example: the test suite for my feature covers cases x, y, and z - [ ] example: all tests pass with my change - [ ] example: I check if my changes works with both PySide and PyQt backends as there are small differences between the two Qt bindings. - [ ] My PR is the minimum possible work for the desired functionality - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] If I included new strings, I have used `trans.` to make them localizable. For more information see our [translations guide](https://napari.org/developers/translations.html).
Fixes/Closes
Closes #5724
Description
References
Type of change
How has this been tested?
as there are small differences between the two Qt bindings.
Final checklist:
trans.
to make them localizable.For more information see our translations guide.