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.
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
{full,zeros,ones}_like typing #6611
{full,zeros,ones}_like typing #6611
Changes from all commits
d080759
b41eef3
c8cc50d
e04a819
44cefd4
a2da478
0164403
3a6ef15
da5bd9b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
For these ones which are generic over
DataArray
andVariable
, we could define aTypeVar
for them (but no need in this PR, and I'm not even sure whether it's a broader issue than just the*_like
.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.
Do you know why this overload is required? I had thought that
DataArray
couldn't take aDTypeMaybeMapping
? (and same principle for the next 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.
I tried to adjust these locally. An issue I hit is that so much of our typing is on
Dataset
rather thanT_Dataset
and changing one seems to require changing its dependencies, which makes it difficult to change gradually. 20 minutes into me trying to change this, I had 31 errors in 6 files!To the extent you know which parts are Perfect vs Not-perfect-but-required-to-pass: If you want to add a comment for the ones the latter, that will make it easier for future travelers to know why things are as they are and hopefully change them.
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 was struggling a lot with
TypeVar
definitions.Some other functions (like
polyval
) are callingzeros_like
withDataArray | Dataset
.This means that a "simple"
TypeVar["T", DataArray, Dataset]
will not work.I tried using
TypeVar["T", bound=DataArray|Dataset]
(recommended by some mypy people) but then theif isinstance(x Dataset)
were causing problems (still not sure if that is a mypy bug or intended, my TypeVar knowledge is not good enough for that)...So this was the only solution I could get to work.
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.
Additionally, the code will actually create a plain e.g. DataArray, so typevars with bounds are actually wrong 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.
Yeah. I think the proposed code is a big upgrade, and we can refine towards perfection in the future...
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.
FWIW I found this SO answer helpful in clarifying the difference — i saw that I had upvoted it before — but I'm still not confident in how we should design these methods.
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.
This is the kind of func that would be nice at some point to make generic; with the proposed code we lose whether it's a
Dataset
vs.DataArray
. (fine to add as a comment / TODO tho)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 failed to make it work with Typevars since it is called with
DataArray|Dataset
:(