Skip to content
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

Typing: Fully type 5 more files #6361

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Oct 22, 2023

This updates a number of files to be fully typed,

I'll note that some of the dynamic nature of napari (like all the proxy stuff) cannot be well types as we can't express the typing informations yet.

I keep this small for quick/easy review, and hopping to decrease the number fo conflicts.

@Carreau Carreau added maintenance PR with maintance changes, typing PR that focus on typing improvement labels Oct 22, 2023
@github-actions github-actions bot added the qt Relates to qt label Oct 22, 2023
@Carreau Carreau force-pushed the more-typing-III branch 4 times, most recently from 2372fb3 to 84dad07 Compare October 22, 2023 10:44
@Carreau
Copy link
Contributor Author

Carreau commented Nov 8, 2023

restarting test.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7405c53) 92.23% compared to head (c03d3e3) 92.20%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6361      +/-   ##
==========================================
- Coverage   92.23%   92.20%   -0.03%     
==========================================
  Files         601      601              
  Lines       53231    53398     +167     
==========================================
+ Hits        49097    49236     +139     
- Misses       4134     4162      +28     

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

@Carreau Carreau force-pushed the more-typing-III branch 2 times, most recently from 823949c to 252dbde Compare November 13, 2023 10:13
@Carreau Carreau force-pushed the more-typing-III branch 4 times, most recently from 21e6dcf to e328c25 Compare November 21, 2023 14:14
@andy-sweet
Copy link
Member

I merged #6356, so you should be able to merge in its commit from main, resolve conflicts, then mark this as ready for review.

@Carreau Carreau marked this pull request as ready for review November 21, 2023 19:11
@Carreau Carreau changed the title Typing: Even more typing (on top of #6352 and #6356) Typing: Fully type 5 more files Nov 21, 2023
@Carreau
Copy link
Contributor Author

Carreau commented Nov 21, 2023

I merged #6356, so you should be able to merge in its commit from main, resolve conflicts, then mark this as ready for review.

Thanks, I rebased, as there are a few PRs on top of this one, and merging tend to make things even more complicated.

@Carreau Carreau added this to the 0.5 milestone Nov 21, 2023
if dtype_ == np.uint16 and dtype.itemsize > 2:
dtype_ = np.float32
dtype_ = np.float32 # type: ignore [assignment]
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this necessary btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imho a problem in mypy type inference:

error: Incompatible types in assignment (expression has type "type[floating[Any]]", variable has type "type[dtype[Any]]") [assignment]

I tried various combinaison above, to no success, and AFAIC, I dont se why X[floating[Y]] can't be assinged to X[dtype[Y]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait, maybe I can try a union. Also maybe we can refactor the try-except with dtype.kind in 'ifub' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, find a way to do it with a union. That does not get rid of the cast, but it's better. thanks

fix types of utils/naming

type napari/utils/info.py

update types in napari/utils/_proxies.py

fully type napari/layers/utils/plane.py

fully type napari/_vispy/utils/gl.py

typalias

refix napari/utils/naming.py

test typing extension on 3.8/3.9

typo

fix type

remove double define

to str as we can't subscript on 3.8

fix types
Copy link
Member

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

Sorry this has been approved and open for a while. I almost just clicked the merge button, but I noticed a couple of things that didn't make sense. Happy to expedite this getting merged to unblock other PRs once those questions are resolved.

napari/_vispy/utils/gl.py Show resolved Hide resolved
@@ -370,6 +370,7 @@ module = [
"napari.layers.utils.interactivity_utils",
"napari.layers.vectors._vector_utils",
"napari.resources._icons",
"napari.utils.color",
Copy link
Member

Choose a reason for hiding this comment

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

Why was this added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typing can have ripple effects, as if mypy sometime achieve to infer further it finds issues. It is likely to be fixed in one of my subsequents PRs relying on this one that are drafts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular in this case:

(many of....): error: Variable "napari._pydantic_compat.Color" is not valid as a type  [valid-type]
(many of....): note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
...
napari/utils/theme.py:141: error: Item Color? of str | Color? has no attribute "startswith"  [union-attr]
napari/utils/theme.py:142: error: Item Color? of str | Color? has no attribute "lstrip"  [union-attr]

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying!

napari/_vispy/utils/gl.py Outdated Show resolved Hide resolved
Copy link
Member

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

This has been approved for a while, is very safe, and blocks other PRs, so I'm going to merge this now instead of doing the usual ready to merge + 24 hours.

@andy-sweet andy-sweet merged commit 6e0d23a into napari:main Dec 4, 2023
32 checks passed
@Carreau
Copy link
Contributor Author

Carreau commented Dec 4, 2023 via email

@andy-sweet andy-sweet modified the milestones: 0.5, 0.5.0 Dec 4, 2023
@Carreau
Copy link
Contributor Author

Carreau commented Dec 5, 2023

Also from this comment I assumed that we would hold any other merge, so should this have been merged ?

@Czaki ?

@Czaki
Copy link
Collaborator

Czaki commented Dec 5, 2023

This PR is not touching files that I'm waiting for someone to review.

But every merged PR with typing increases hardness of cherry-picking, so increases some probability that some bug will be introduced.

@andy-sweet
Copy link
Member

This PR is not touching files that I'm waiting for someone to review.

But every merged PR with typing increases hardness of cherry-picking, so increases some probability that some bug will be introduced.

Sorry! I didn't see that message. Will hold off on merges over the next week or so, or until the release seems to have stabilized.

@Carreau
Copy link
Contributor Author

Carreau commented Dec 5, 2023

No worries, it is also for me to understand what need to be done, as right now I feel 0.4.19 is starting to seriously impair work on the main branch. So maybe we need everyone to focus on 0.4.19 and push it over the line.

@Czaki
Copy link
Collaborator

Czaki commented Dec 5, 2023

I have hope that only one PR is left for 0.4.19 #6439. But it touches fragile part of code (rendering). And do not have big number of reviewing core-devs

@Carreau
Copy link
Contributor Author

Carreau commented Dec 5, 2023

I still see a few things tagged 0.4.19, 4prs, 2 PRs drafts and 2 issues: https://github.com/napari/napari/milestone/38, should they be retagged ?

@Czaki
Copy link
Collaborator

Czaki commented Dec 5, 2023

2 of these PR are against 0.4.19, so no cherry-picking problems. Two drafts, are an alternative solution for a single problem that requires #6439 to be merged. Then it could be quickly solved.

kne42 added a commit to kne42/napari that referenced this pull request Dec 20, 2023
* main: (80 commits)
  Check in LabelColormap that fewer than 2**16 colors are requested (napari#6540)
  Fix label color shuffling by also updating colormap size (napari#6460)
  Add `_block_refresh()` to layers (napari#6525)
  MNT: Use `partial` in samples menu to avoid leaking  (napari#6538)
  Update performance and reduce memory usage for big Labels layer in direct color mode (napari#6439)
  Reset single step and decimals on reset range slider in popup (napari#6523)
  Add copy operator to fix memory benchmarks (napari#6530)
  Restore quit shortcut (napari#6526)
  Fix problem with invalidate cache  (napari#6520)
  [pre-commit.ci] pre-commit autoupdate (napari#6505)
  Pass key event from Main window to our internal mechanism (napari#6419)
  Typing: Fully type 5 more files (napari#6361)
  Do not use native dialog for reset shortcuts (napari#6493)
  Use views rather than CPU-based hashing for 8- and 16-bit Labels data (napari#6467)
  Add import lint back to CI (napari#6506)
  Type napari.layers.image helper sub-modules (napari#6499)
  Bugfix: ensure gamma and opacity are floats (napari#6498)
  FIX: Remove `None` default from `_remove_dock_widget` (napari#6481)
  Add testing_extra and optional dependencies when creating constraints (napari#6487)
  Run test suite with optional dependencies and fix tests when `triangle` is installed (napari#6488)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PR with maintance changes, qt Relates to qt typing PR that focus on typing improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants