Return numpy.datetime64 arrays for non-standard calendars #126
Conversation
…o support virtual_variable indexing, #121
…64 object to valid years (1677<time<2262)
| def nctime_to_nptime(times): | ||
| """Given an array of netCDF4.datetime objects, return an array of | ||
| numpy.datetime64 objects""" | ||
| new = np.empty(len(times), dtype='M8[ns]') |
There was a problem hiding this comment.
Could you please make this work for arrays with arbitrary shape?
You would need to replace len(times) with times.size and then do a reshape at the end to times.shape.
…e ability to fallback to netCDF4.datetime objects since this solution does not fix the problem for all calendars in all situations.
| 'numpy.datetime64 objects, continuing using ' | ||
| 'dummy netCDF4.datetime objects instead', | ||
| RuntimeWarning, stacklevel=2) | ||
| pass |
There was a problem hiding this comment.
pass is no-op here -- you can safely remove it.
|
The issue with the It would also be nice if you could also issue a warning when we fall back to netCDF4 datetime objects because there are dates outside the valid range. The In terms of tests:
|
| warnings.warn('Unable to decode time axis into full ' | ||
| 'numpy.datetime64 objects, continuing using ' | ||
| 'dummy netCDF4.datetime objects instead, reason:' | ||
| '{0}'.format(e), RuntimeWarning, stacklevel=2) |
There was a problem hiding this comment.
It is indeed better to issue a single warning -- that way it can be filtered more easily.
Expanded warnings for when non numpy.datetime64 arrays are returned. Use context manager for warnings in test_conventions.py.
|
I think this is all ready to go. In the future and maybe as a separate project, I'd like to see more complete non-Gregorian calendar support in Python. I know the climate modeling community would utilize the feature. For now however, I think this is a good step for xray. |
| @requires_netCDF4 | ||
| def test_decode_non_standard_calendar_single_element(self): | ||
|
|
||
| for calendar in ['noleap', '365_day', '360_day', 'julian', 'all_leap', |
There was a problem hiding this comment.
My preference would be to the test the different calendars separately, based on whether they can be decoded into datetime64 or not. That way you can verify expected results exactly (e.g., the array dtype and whether they raise a warning or not). The current test leaves some room for ambiguity:).
There was a problem hiding this comment.
I've written a separate test to make sure we get what we're expecting for single element fallbacks.
…uring fallback of single element
Return numpy.datetime64 arrays for non-standard calendars
|
Thanks you, Joe! |
|
I just closed them. There is actually a cool feature of GitHub where if you write your commit message in the form "Fixes #118" the issue gets closed automatically upon merging: https://help.github.com/articles/closing-issues-via-commit-messages |
Fixes issues in #118 and #121