Fix #665 decode_cf_timedelta 2D #842
Conversation
| @@ -165,11 +165,18 @@ def decode_cf_timedelta(num_timedeltas, units): | |||
| """ | |||
| num_timedeltas = _asarray_or_scalar(num_timedeltas) | |||
| units = _netcdf_to_numpy_timeunit(units) | |||
|
|
|||
| shape = None | |||
| if isinstance(num_timedeltas, np.ndarray): | |||
There was a problem hiding this comment.
I would unilaterally convert to a numpy array: num_timedeltas = np.asarray(num_timedeltas). Then save the shape and ravel. Note that .ravel() is basically free for 1D data since it only makes a copy if necessary.
There was a problem hiding this comment.
I can change it. The only reason I did this way was to preserve the _asarray_or_scalar(x) behavior from above.
There was a problem hiding this comment.
I'm not 100% sure, but I think the _asarray_or_scalar is there because pandas's to_timedelta doesn't like receiving 0-dimensional numpy arrays as input. As long as you're flattening everything you pass to to_timedelta I think arrays should always be OK. Tests should tell us either way.
There was a problem hiding this comment.
I'm not 100% sure, but I think the
_asarray_or_scalaris there because pandas'sto_timedeltadoesn't like receiving 0-dimensional numpy arrays as input.
If that is the only reason it should go away. How does it look now?
|
Looks great! Please add a brief bug fix note to "What's new", then I will merge. |
How about 518ea53? PS: @shoyer I am still not sure that converting any data that has units of time to |
| @@ -42,6 +42,9 @@ Bug fixes | |||
| ``keep_attrs=True`` option. By | |||
| `Jeremy McGibbon <https://github.com/mcgibbon>`_. | |||
|
|
|||
| - ``decode_cf_timedelta`` now accepts arrays with ``ndim`` >1 (pydata/xarray/pull/842). | |||
There was a problem hiding this comment.
don't forget to add your name in the credits!
also, reference the issue like this:
:issue:`842`
|
I agree that decoding time units to timedeltas is not always desirable -- possibly we should add an explicit toggle for decoding timedeltas vs datetimes. |
ocefpaf
force-pushed
the
decode_cf_timedelta
branch
from
518ea53 to
888e6c2
Compare
👍 I am opening a separated issue for that. |
|
Thanks @ocefpaf ! |
|
Sorry, I'm going camping this weekend so I won't be able to get this out.
|
No biggie. As I mentioned above I have a plan B (conda install of the latest dev version). Enjoy your camping! |
Long time listener first time caller 😉
I am not 100% about this PR though. I think that there are cases when we need the actual data rather than the
timedelta. In this notebook we have wave period ('mper') that should be seconds ranging0-30and not those big numpy timedelta numbers.I know that I can get that behavior with
decode_times=Truewhen opening the dataset, but then thetimecoordinate get decoded as well. (Maybe I am way off and there is a way to do this.)