Skip to content

Don't convert data with time units to timedeltas by default #843

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

Closed
ocefpaf opened this issue May 4, 2016 · 6 comments
Closed

Don't convert data with time units to timedeltas by default #843

ocefpaf opened this issue May 4, 2016 · 6 comments

Comments

@ocefpaf
Copy link
Contributor

ocefpaf commented May 4, 2016

Don't convert data with time units to timedeltas by default. Most of the time this behavior is not desirable (e.g: wave period data).

@shoyer suggest:

possibly we should add an explicit toggle for decoding timedeltas vs datetimes.

xref: #842 (comment)

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 4, 2016

@shoyer is this what you have in mid? Replacing decode_time for decode_datetimes and decode_timedelta and then:

    if decode_datetimes and 'units' in attributes and 'since' in attributes['units']:
        units = pop_to(attributes, encoding, 'units')
        calendar = pop_to(attributes, encoding, 'calendar')
        data = DecodedCFDatetimeArray(data, units, calendar)

    if decode_timedelta and attributes['units'] in TIME_UNITS:
        units = pop_to(attributes, encoding, 'units')
        data = DecodedCFTimedeltaArray(data, units)

To be honest I cannot see why someone might want to convert time data to a timedelta and I would be inclined to remove decode_timedelta instead. However, that may be my very biased view since such transformation does not make sense with my data 😄

Do you have any advice? (I would like to try this before you release v0.8.0)

@shoyer
Copy link
Member

shoyer commented Aug 4, 2016

It's too late for v0.8.0, I'm afraid! I already tagged/uploaded the release and am about to send out the announcement.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 4, 2016

No problem. I want to solve this anyways b/c it is quite awkward to keep converting back from timedeltas to time data. I will have a PR soon.

@shoyer
Copy link
Member

shoyer commented Aug 4, 2016

To answer your original question: yes, separate decode_timedeltasand decode_datetimes options seems like a sane way to handle this.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 4, 2016

Prototype is ready http://nbviewer.jupyter.org/gist/ocefpaf/cabdcb27a5ef7da1b6110327a5f03e17

I will polish this and send a PR soon.

@ocefpaf
Copy link
Contributor Author

ocefpaf commented Aug 11, 2016

@shoyer the more I think about this the more I don't like the addition of extra keywords. Even though I would like this behavior to be the default one I really do not like the complexity I added here.

Closing this...

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

No branches or pull requests

2 participants