BUG: Fixes GH3215 #3220
BUG: Fixes GH3215 #3220
Conversation
Explicit cast to numpy array to avoid np.ravel calling out to dask
|
Hi @aidanheerdegen - apologies no one followed up on this for a few days. Thank you for the contribution! Could you add a test? |
|
HI @max-sixty. I am working on making a test, but when I serialise my test file so it is suitable for inclusion in a test it doesn't throw an error! Seems |
|
I can save the decoded version to a file and read it back in and it throws the error. I suppose this is traversing a different code path |
|
When you write a dataset to a dict, the dask arrays are converted into NumPy arrays. Try something like |
|
Hello @aidanheerdegen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-08-28 06:29:45 UTC |
|
Thanks @shoyer. I have added a test. It contains no assertion, but does fail with without the code update. Is that sufficient? |
xarray/tests/test_conventions.py
Outdated
| ) | ||
| assert_identical(decoded, conventions.decode_cf(original).compute()) | ||
|
|
||
| def test_experimental_array(self): |
There was a problem hiding this comment.
Maybe call this test_decode_dask_times?
There was a problem hiding this comment.
You also will need to use the @requires_dask decorator here, so this test only gets run if dask is installed.
xarray/tests/test_conventions.py
Outdated
| "average_T1": { | ||
| "dims": ("time",), | ||
| "attrs": { | ||
| "long_name": "Start time for average period", |
There was a problem hiding this comment.
do you need this attribute? if not, please remove it to keep the test example as simple as possible.
xarray/tests/test_conventions.py
Outdated
| ) | ||
| # Throws an AttributeError: 'Array' object has no attribute 'tolist' | ||
| # without updated code | ||
| conventions.decode_cf(original.chunk()) |
There was a problem hiding this comment.
It is definitely preferrable to construct a positive example of what the dataset should be (and use assert_identical to verify it), but this is certainly sufficient that we don't get an error anymore.
There was a problem hiding this comment.
Doesn't work to assert_equal to a non-dask version (I tried). pytest supports context managers when an assertion is expected, but not when one isn't. As I said, I could wrap in a try/except but not sure what that will achieve.
I've added an assert test that serialises the dask and non-dask datasets. Hopefully that suffices.
There was a problem hiding this comment.
What happens if you write:
expected = conventions.decode_cf(original)
actual = conventions.decode_cf(original.chunk())
assert_identical(expected, actual) If that doesn't work, you could also try:
expected = conventions.decode_cf(original)
actual = conventions.decode_cf(original.chunk()).compute()
assert_identical(expected, actual) There was a problem hiding this comment.
Added a valid assert_identical test which I think should do the trick.
There was a problem hiding this comment.
Sorry, crossed comments. Your first suggestion throws this:
> elif hasattr(a, "data") and type(a).__module__.startswith("xarray."):
E ValueError: cannot include dtype 'M' in a buffer
I think the test I just added may be quite close to your second suggestion, but flipped, so the "truth" is coerced to a dask array before comparison.
There was a problem hiding this comment.
Interesting, that is definitely a dask bug. Should be fixed by dask/dask#5334.
This version looks fine.
…jects. `da.asanyarray()` current crashes if passed a datetime64 dtype array, as noted in pydata/xarray#3220: > elif hasattr(a, "data") and type(a).__module__.startswith("xarray."): E ValueError: cannot include dtype 'M' in a buffer Reversing the order of these checks fixes the issue. I also adjusted `da.asarray()` to coerce xarray objects in the same way as `da.asanyarray()`.
…jects. `da.asanyarray()` current crashes if passed a datetime64 dtype array, as noted in pydata/xarray#3220: > elif hasattr(a, "data") and type(a).__module__.startswith("xarray."): E ValueError: cannot include dtype 'M' in a buffer Reversing the order of these checks fixes the issue. I also adjusted `da.asarray()` to coerce xarray objects in the same way as `da.asanyarray()`.
|
thanks @aidanheerdegen ! |
…jects (#5334) * Fixup da.asarray and da.asanyarray for datetime64 dtype and xarray objects. `da.asanyarray()` current crashes if passed a datetime64 dtype array, as noted in pydata/xarray#3220: > elif hasattr(a, "data") and type(a).__module__.startswith("xarray."): E ValueError: cannot include dtype 'M' in a buffer Reversing the order of these checks fixes the issue. I also adjusted `da.asarray()` to coerce xarray objects in the same way as `da.asanyarray()`. * Update dask/array/tests/test_xarray.py Co-Authored-By: Matthew Rocklin <[email protected]>
4 participants
Explicit cast to numpy array to avoid np.ravel calling out to dask