BUG: Avoid undefined behaviour when converting from float to timedelta #28918
Conversation
is there a test that would fail currently on our CI and pass with this patch? or is this just platform specific? |
It looks like all of your CI tests are x86-only, so you won't see this issue. If you run the testsuite on aarch64 though, you'll see |
pandas/core/nanops.py
Outdated
| @@ -360,7 +360,7 @@ def _wrap_results(result, dtype, fill_value=None): | |||
|
|
|||
| result = tslibs.Timedelta(result, unit="ns") | |||
| else: | |||
| result = result.astype("i8").view(dtype) | |||
| result = result.astype("m8").view(dtype) | |||
There was a problem hiding this comment.
would making this m8[ns] break things?
There was a problem hiding this comment.
It shouldn't. I've started a test run on my arm box with it.
Do azure or travis offer such a thing? (Or anyone else?) |
I believe Travis does now, as of just 4 days ago: https://blog.travis-ci.com/2019-10-07-multi-cpu-architecture-support |
Summation of timedelta series with NaTs in them result in undefined behaviour because the final wrapping step of the summation ends up converting the NaNs in the sum through a direct cast to int64. This cast is undefined for NaN and just happens to work on x86_64 because of the way cvttd2si works. On Aarch64, the corresponding fcvtzs sets the result to 0 on undefined input. This fix trivially sets the conversion target to m8 instead of i8 so that numpy correctly casts from NaN to NaT. Note that the fix in numpy for the same is pending in PR #numpy/numpy/14669 . There is an existing test (test_sum_nanops_timedelta in frame/test_analytics.py) that exercises this bug and has been verified to have been fixed with this and the numpy patch.
|
thanks @siddhesh would be interested in adding a travis build on arm? (can you open a separate issue to track). |
pandas-dev#28918) Summation of timedelta series with NaTs in them result in undefined behaviour because the final wrapping step of the summation ends up converting the NaNs in the sum through a direct cast to int64. This cast is undefined for NaN and just happens to work on x86_64 because of the way cvttd2si works. On Aarch64, the corresponding fcvtzs sets the result to 0 on undefined input. This fix trivially sets the conversion target to m8 instead of i8 so that numpy correctly casts from NaN to NaT. Note that the fix in numpy for the same is pending in PR #numpy/numpy/14669 . There is an existing test (test_sum_nanops_timedelta in frame/test_analytics.py) that exercises this bug and has been verified to have been fixed with this and the numpy patch.
pandas-dev#28918) Summation of timedelta series with NaTs in them result in undefined behaviour because the final wrapping step of the summation ends up converting the NaNs in the sum through a direct cast to int64. This cast is undefined for NaN and just happens to work on x86_64 because of the way cvttd2si works. On Aarch64, the corresponding fcvtzs sets the result to 0 on undefined input. This fix trivially sets the conversion target to m8 instead of i8 so that numpy correctly casts from NaN to NaT. Note that the fix in numpy for the same is pending in PR #numpy/numpy/14669 . There is an existing test (test_sum_nanops_timedelta in frame/test_analytics.py) that exercises this bug and has been verified to have been fixed with this and the numpy patch.
pandas-dev#28918) Summation of timedelta series with NaTs in them result in undefined behaviour because the final wrapping step of the summation ends up converting the NaNs in the sum through a direct cast to int64. This cast is undefined for NaN and just happens to work on x86_64 because of the way cvttd2si works. On Aarch64, the corresponding fcvtzs sets the result to 0 on undefined input. This fix trivially sets the conversion target to m8 instead of i8 so that numpy correctly casts from NaN to NaT. Note that the fix in numpy for the same is pending in PR #numpy/numpy/14669 . There is an existing test (test_sum_nanops_timedelta in frame/test_analytics.py) that exercises this bug and has been verified to have been fixed with this and the numpy patch.
Summation of timedelta series with NaTs in them result in undefined
behaviour because the final wrapping step of the summation ends up
converting the NaNs in the sum through a direct cast to int64. This
cast is undefined for NaN and just happens to work on x86_64 because
of the way
cvttd2siworks. On Aarch64, the correspondingfcvtzssetsthe result to 0 on undefined input.
This fix trivially sets the conversion target to m8 instead of i8 so
that numpy correctly casts from NaN to NaT. Note that the fix in
numpy for the same is pending in PR numpy/numpy#14669 .
There is an existing test (test_sum_nanops_timedelta in
frame/test_analytics.py) that exercises this bug and has been verified
to have been fixed with this and the numpy patch.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff