xarray
xarray copied to clipboard
pydata
Don't cast NaN to integer
Do not try to convert NaN to integer type, as the operation is undefined and results in random values. This fixes all testsuite failures.
Hi @andreas-schwab, and welcome to xarray!
Can you tell what specific failures this change fixed for you? If there was something failing we want to capture it in our test suite, but I am not sure what failure you are referring to.
It's already perfectly covered by the testsuite.
=========================== short test summary info ============================ FAILED xarray/tests/test_backends.py::TestScipyInMemoryData::test_roundtrip_numpy_datetime_data FAILED xarray/tests/test_backends.py::TestScipyFileObject::test_roundtrip_numpy_datetime_data FAILED xarray/tests/test_backends.py::TestGenericNetCDFData::test_roundtrip_numpy_datetime_data FAILED xarray/tests/test_backends.py::TestScipyFilePath::test_roundtrip_numpy_datetime_data = 4 failed, 4636 passed, 5632 skipped, 19 xfailed, 22 xpassed, 38 warnings in 266.18s (0:04:26) =
It's already perfectly covered by the testsuite.
Okay great, but then why are these tests failing for you (locally?) and not in our CI runs? (Our main branch just passed all automated tests just now.) Do you have a different version of some package that we aren't testing against?
I'm asking because if our current CI test runs don't reproduce this error, then we (a) have no way to check that your change fixed the error and (b) will not know if some regression causes this error to resurface in future. Standard practice would be to raise an issue that demonstrates the problem, then link the fix PR to that issue.
That's the beauty of undefined behaviour. It comes in many flavors, and you never know which one you get. Which also means that it is impossible to reliably test for it.
@TomNicholas Something different will need to happen with that cast eventually. See #6191 for something that is failing on some systems that users have but is currently unable to be captured in the tests. Numpy has already added runtime warnings about doing this, and is "thinking about" making nan to int casts raise https://github.com/numpy/numpy/issues/14412. Xarray's own @shoyer has hit issues like this before as well https://github.com/numpy/numpy/issues/6109.
I think the real solution here is to explicitly handle NaNs during the decoding step. We do want these to be NaT in the output.
cc @spencerclark
Hi, is there any reason that keeps this pr from being merged? This PR is a fix for some failing tests on riscv64.
@andreas-schwab Thanks again for this PR. It turned out it was a bit more involved to make this work and we hope #7827 solves the underlying issue.
