Fix maybe_promote #1953
Fix maybe_promote #1953
Conversation
With tests for every possible dtype:
(numpy docs say `biufcmMOSUV` only)
```
for letter in string.ascii_letters:
try:
print(letter, np.dtype(letter))
except TypeError as exc:
pass
```
xarray/core/dtypes.py
Outdated
| @@ -78,7 +78,9 @@ def maybe_promote(dtype): | |||
| fill_value : Valid missing value for the promoted dtype. | |||
| """ | |||
| # N.B. these casting rules should match pandas | |||
| if np.issubdtype(dtype, np.floating): | |||
| if np.issubdtype(dtype, np.timedelta64): | |||
There was a problem hiding this comment.
can you add a comment with a link to the numpy bug explaining why we need to do this first?
There was a problem hiding this comment.
Also maybe move this below np.floating -- there was an intention to hit more commonly used dtypes first.
xarray/tests/test_dtypes.py
Outdated
| actual = dtypes.maybe_promote(np.dtype(kind)) | ||
| if np.issubdtype(np.dtype(kind), np.complexfloating) or kind in 'mM': | ||
| assert actual[0] == expected[0] | ||
| assert str(actual[1]) == str(expected[1]) |
There was a problem hiding this comment.
Instead of keeping things as tuples, can we split actual/expected into values and dtypes? This would make things a little clearer:
assert actual_dtype == expected_dtype
assert str(actual_value) == str(expected_value)
xarray/tests/test_dtypes.py
Outdated
| assert actual[0] == expected[0] | ||
| assert str(actual[1]) == str(expected[1]) | ||
| else: | ||
| assert actual == expected |
There was a problem hiding this comment.
It is probably better not to rely on this code-path ever working -- it relies on quirks of tuples using object identity in comparisons in a way that is arguably broken for NaN.
There was a problem hiding this comment.
OK, I skipped all complex dtypes.
xarray/tests/test_dtypes.py
Outdated
| ('F', (np.complex64, np.complex('nan+nanj'))), # dtype('complex64') | ||
| ('f', (np.float32, np.nan)), # dtype('float32') | ||
| ('G', (np.complex256, np.complex('nan+nanj'))), # dtype('complex256') | ||
| ('g', (np.float128, np.nan)), # dtype('float128') |
There was a problem hiding this comment.
Apparently complex256 and float128 aren't always available -- that's why our integration tests on Appveyor (running Windows) fail for this PR. So let's exclude these.
There was a problem hiding this comment.
I ignored float128 (and complex256, see #1953 (comment))
|
@NotSqrt this is really close -- any interest in finishing this up? :) |
In order to hit this branch more often
|
@shoyer Can you review those changes, please ? Thanks ! |
|
thanks @NotSqrt |
With tests for every possible dtype:
(numpy docs say
biufcmMOSUVonly)whats-new.rstfor all changes andapi.rstfor new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)