Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Time decoding has round-off error 0.10.0. Gone now. #1859

Closed
j08lue opened this issue Jan 25, 2018 · 3 comments · Fixed by #1863
Closed

Time decoding has round-off error 0.10.0. Gone now. #1859

j08lue opened this issue Jan 25, 2018 · 3 comments · Fixed by #1863

Comments

@j08lue
Copy link
Contributor

j08lue commented Jan 25, 2018

Note: This problem occurs with version 0.10.0, but is gone when using current master (0.10.0+dev44.g0a0593d).

Here is a complete example:
https://gist.github.com/j08lue/34498cf17b176d15933e778278ba2921

Problem description

I have this time variable from a netCDF file:

float32 time(time)
    units: days since 1980-1-1 0:0:0
    standard_name: time
    calendar: gregorian

And when I open the file with xr.open_dataset(..., decode_times=True) the first time stamp becomes 1982-12-31T23:59:59.560122368, while it should be 1983-01-01. For reference, netCDF4.num2date gets it right.

I tracked the problem down to xarray.conventions.decode_cf_datetime. But then also noticed that you in 50b0a69 made major changes to the decoding.

So I also tested with current master (0.10.0+dev44.g0a0593d) and the problem is gone.

Up to you what you make of this. 😄 Maybe you can just close the issue.

Output of xr.show_versions()

INSTALLED VERSIONS ------------------ commit: None python: 3.6.4.final.0 python-bits: 64 OS: Windows OS-release: 10 machine: AMD64 processor: Intel64 Family 6 Model 60 Stepping 3, GenuineIntel byteorder: little LC_ALL: None LANG: ZZ LOCALE: None.None

xarray: 0.10.0
pandas: 0.21.1
numpy: 1.13.3
scipy: 1.0.0
netCDF4: 1.3.1
h5netcdf: 0.5.0
Nio: None
bottleneck: 1.2.1
cyordereddict: 1.0.0
dask: 0.16.1
matplotlib: 2.1.1
cartopy: None
seaborn: None
setuptools: 38.2.4
pip: 9.0.1
conda: None
pytest: 3.3.1
IPython: 6.2.1
sphinx: None
In [4]:

@shoyer
Copy link
Member

shoyer commented Jan 25, 2018

Interesting. This definitely wasn't an intended fix, though, so it would probably still be a good idea to add a test-case covering this behavior so we don't have a regression (there are plans to refactor the encoding/decoding module more in the future). Any interesting in putting together a PR with a test?

@j08lue
Copy link
Contributor Author

j08lue commented Jan 26, 2018

PR with a test

I'll see if I can find the time in the next few days.

@j08lue
Copy link
Contributor Author

j08lue commented Jan 28, 2018

I found out why the issue is absent in the current implementation. Please see #1863 (comment). That PR adds the test you asked for @shoyer. It passes in the current version and fails in v0.10.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants