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

Add use_cftime option to open_dataset #2759

Merged
merged 6 commits into from
Feb 19, 2019

Conversation

spencerkclark
Copy link
Member

@spencerkclark spencerkclark commented Feb 11, 2019

Based on @shoyer's suggestion in #2754 (comment).

if calendar in _STANDARD_CALENDARS:
warnings.warn(
'Unable to decode time axis into full '
'numpy.datetime64 objects, continuing using dummy '
Copy link
Collaborator

@mathause mathause Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not call them 'dummy' anymore?

Suggested change
'numpy.datetime64 objects, continuing using dummy '
'numpy.datetime64 objects, continuing using '

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a good idea.

@shoyer
Copy link
Member

shoyer commented Feb 12, 2019

Let me suggest a slight variation: let's make the behavior of use_cftime=None dependent on the calendar attribute, e.g.,

  • gregorian, standard, proleptic_gregorian or not found: try to decode into numpy.datetime64
  • everything else: only use cftime -- numpy.datetime64 is not correct!

@spencerkclark
Copy link
Member Author

As of #2516 this is already the default behavior :)

@shoyer
Copy link
Member

shoyer commented Feb 13, 2019

As of #2516 this is already the default behavior :)

OK, great! Maybe we should just update the description of use_cftime, to clarify that it only is relevant with standard calendars?

@spencerkclark
Copy link
Member Author

Maybe we should just update the description of use_cftime, to clarify that it only is relevant with standard calendars?

Good idea! See my update in 52866a8.

@dcherian dcherian mentioned this pull request Feb 19, 2019
@@ -397,15 +407,17 @@ def encode(self, variable, name=None):

return Variable(dims, data, attrs, encoding)

def decode(self, variable, name=None):
def decode(self, variable, name=None, use_cftime=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep this option on the __init__ method instead? That’s what we’ve done for other coder objects.

I think that is a little nicer since it makes it more obvious how to use coders.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. That is nicer.

Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Feel free to merge this if you're happy with it!

@spencerkclark
Copy link
Member Author

Ok merging now! Thanks @shoyer for your initial design suggestion.

@spencerkclark spencerkclark merged commit 612d390 into pydata:master Feb 19, 2019
@spencerkclark spencerkclark deleted the use-cftime-option branch February 19, 2019 20:47
dcherian added a commit to yohai/xarray that referenced this pull request Mar 18, 2019
* upstream/master:
  Rework whats-new for 0.12
  Add whats-new for 0.12.1
  Release 0.12.0
  enable loading remote hdf5 files (pydata#2782)
  Push back finalizing deprecations for 0.12 (pydata#2809)
  Drop failing tests writing multi-dimensional arrays as attributes (pydata#2810)
  some docs updates (pydata#2746)
  Add support for cftime.datetime coordinates with coarsen (pydata#2778)
  Don't use deprecated np.asscalar() (pydata#2800)
  Improve name concat (pydata#2792)
  Add `Dataset.drop_dims` (pydata#2767)
  Quarter offset implemented (base is now latest pydata-master). (pydata#2721)
  Add use_cftime option to open_dataset (pydata#2759)
  Bugfix/reduce no axis (pydata#2769)
  'standard' now refers to 'gregorian' in cftime_range (pydata#2771)
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 this pull request may close these issues.

3 participants