Cast datetime and timedelta to signed 64-bit int #344
Conversation
|
Pushed another test. It is a bit broken still, but added it anyways as I could use some advice on how to fix it. Edit: Should be fixed with the latest commit. |
| assert np.all( | ||
| fill_value.view((np.uint8, fill_value.itemsize)) == | ||
| meta_dec['fill_value'].view((np.uint8, meta_dec['fill_value'].itemsize)) | ||
| ) |
There was a problem hiding this comment.
As NaT values may compare False (like NaN values) in the future, this is a workaround to handle that case while still making such comparisons True. The itemsize needs to be included for this to work on 0-D arrays (e.g. scalars).
|
Please let me know if there is anything else needed here. |
|
Have resolved conflicts and rebased. Should be ready to go once CI clears. |
The `NaT` type is represented as `-0`. As a result, casting to an unsigned integral fails and throws an error. However casting to a signed integral type does not have this problem and proceeds without issues.
Use a structured array with datetime and timedelta values and a fill value of NaT to test a bunch of different workarounds for encoding and decoding datetime and timedelta values and array.
zarr/tests/test_meta.py
Outdated
| for k in ['m8[s]', 'M8[s]']: | ||
| compressor = Blosc(cname='lz4', clevel=3, shuffle=2) | ||
| dtype = np.dtype(k) | ||
| fill_value = np.full((), np.nan, dtype=dtype)[()] |
There was a problem hiding this comment.
Should we use NaT as the fill value, to test it explicitly?
There was a problem hiding this comment.
E.g.:
| fill_value = np.full((), np.nan, dtype=dtype)[()] | |
| fill_value = np.full((), 'NaT', dtype=dtype)[()] |
There was a problem hiding this comment.
Hmm...that doesn't seem to work. 😕
ValueError: invalid literal for int() with base 10: 'NaT'There was a problem hiding this comment.
Pushed a different change that uses NaT explicitly. Please let me know what you think.
There was a problem hiding this comment.
Thanks, looks good. Had a small comment on the metadata test. Also wondering if it would be worth adding a test to the test_creation module, to make sure you can use 'NaT' as a fill_value, e.g.:
z = zarr.full(100, fill_value='NaT', dtype='M8[s]')
assert np.all(np.isnat(z[:]))
|
Should be ready for another look when you have a chance. |
|
Thanks @jakirkham. I added in a test to verify you can pass |
Should fix the flake8 error seen on CI. ref: https://travis-ci.org/zarr-developers/zarr/jobs/465075634
|
Pushed one small change to fix a linting error. Looks like it is passing now. |
|
Awesome, thanks! |
Related to issue ( #342 )
The
NaTtype is represented as-0. As a result, casting to an unsigned integral fails and throws an error. However casting to a signed integral type does not have this problem and proceeds without issues.TODO:
tox -e docs)