-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix static typing with Matplotlib 3.8 #8030
Merged
Merged
Changes from 15 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
fd2c467
fix several typing issues with mpl3.8
headtr1ck a97d634
Merge branch 'main' into pr/8030
Illviljan 63409a5
Merge branch 'main' into mpltypes
headtr1ck 3b62890
fix scatter plot typing and remove funky pyplot import
headtr1ck 7b44b1b
fix some more typing errors
headtr1ck 9051f30
fix some import errors
headtr1ck 55a63b6
undo some typing errors
headtr1ck 9e2c352
fix xylim typing
headtr1ck 42d3356
add forgotten import
headtr1ck b5d2249
ignore plotting overloads because None is Hashable
headtr1ck d1cc216
add whats-new entry
headtr1ck 428a1ed
fix return type of hist
headtr1ck 2ab1402
fix another xylim type
headtr1ck db0db64
fix some more xylim types
headtr1ck 3809152
Merge branch 'main' into mpltypes
headtr1ck 8897ba1
change to T_DataArray
headtr1ck c318096
Merge branch 'main' into mpltypes
headtr1ck 64c2401
change accessor xylim to tuple
headtr1ck 79a92f5
add missing return types
headtr1ck b9dcc6a
fix a typing error only on new mpl
headtr1ck 1eef361
add unused-ignore to error codes for old mpl
headtr1ck f7246fe
add more unused-ignore to error codes for old mpl
headtr1ck 33c6f20
replace type: ignore[attr-defined] with assert hasattr
headtr1ck 06e4337
Merge branch 'main' into mpltypes
headtr1ck b1d721c
apply code review suggestions
headtr1ck File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Have you tried Optional[Hashable] ?
I saw in the NamedArray PR we sometimes use this trick:
I am not sure how scary it would be to just change to this method?
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 don't think one should deviate from None as default if it is preventable.
This issue if exactly this: python/mypy#13805
Not sure why for some methods it is only surfacing now and not already before.
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.
That is exactly the same as
Hashable | None
, and tools like pyupgrade will also replace that.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 guess the question is; does xarray allow
None
as a dim/coordinate? If we do, we have to use other defaults.Considering our insistence to use
Hashable
for dims it seems we want to:I suppose this is just another issue with dim(s) haven't been consistently typed before.
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.
While None is technically hashable probably half of xarrays functions use it as default for dimension arguments, so it would be quite a large breaking change to allow it as a dimension/variable/coordinate "name".
I think it is not worth it to introduce such a breaking change just to allow such a corner case.
If something, we should probably add a check for not None in the constructors of xarray objects.
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.
If you want, you can open a follow up issue for that.
Definitely this is out of scope for this PR.