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
Avoid coercing to numpy in
as_shared_dtypes
#8714base: main
Are you sure you want to change the base?
Avoid coercing to numpy in
as_shared_dtypes
#8714Changes from 1 commit
c6f4e3a
1467c4c
d9931ef
c067f7d
5092aaa
a884ba8
86e6bf8
630629c
45808d8
e625c67
6833d66
bcd02bf
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.
Will this idea always work? What if it steps down through a lazy decoder class that changes the dtype...
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.
Those should be going through
xarray/xarray/coding/variables.py
Lines 52 to 64 in c9ba2be
so you should be fine.
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 think I'm getting confused as to how this all works now... Don't I want to be computing
as_shared_dtype
using the dtype of the outermost wrapped class? Whereas this will step through all the way to the innermost duckarray, which may have a different dtype?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.
As of now,
as_shared_dtype
is expected to return pure duck arrays for stack, concatenate, and where.So that means we need to read from disk, which you do with
to_duck_array
and all these wrapper layers will be resolved.It will get more complicated when we do lazy concatenation in Xarray, then we'd need to lazily infer dtypes and apply a lazy astype.
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.
Previously this
asarray
call would coerce to numpy unnecessarily, when all we really wanted was an array type that we could examine the.dtype
attribute of.Unchanged files with check annotations Beta
Check failure on line 146 in xarray/tests/test_array_api.py
GitHub Actions / ubuntu-latest py3.9 bare-minimum
Check failure on line 146 in xarray/tests/test_array_api.py
GitHub Actions / ubuntu-latest py3.9 min-all-deps
Check failure on line 146 in xarray/tests/test_array_api.py
GitHub Actions / ubuntu-latest py3.10 all-but-dask