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.

Sign up for GitHub

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

Jump to bottom

Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals #3543

Merged
merged 8 commits into from
Dec 7, 2019
Merged

Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals #3543

merged 8 commits into from
Dec 7, 2019

Conversation

spencerkclark
Copy link
Member

@spencerkclark spencerkclark commented Nov 16, 2019 •
edited
Loading

This is a possible fix for the issue @mathause described in #3535 (comment). @TomNicholas does this seem like a safe change to make in combine_by_coords?

@mathause
Copy link
Collaborator

Thanks. Just for reference, the current implementation leads to a series of CFTimeIndex (which fails when subtracting one from the other):

pd.Index([index.take([0]) for index in indexes]).to_series()

>>> [4500-12-31 00:00:00]    CFTimeIndex([4500-12-31 00:00:00], dtype='obje...
>>> [4600-12-31 00:00:00]    CFTimeIndex([4600-12-31 00:00:00], dtype='obje...
>>> [5100-12-31 00:00:00]    CFTimeIndex([5100-12-31 00:00:00], dtype='obje...
>>> dtype: object

The implementation in this PR leads to a series of CFTime (and subtracting one from the other works):

pd.Index([index[0] for index in indexes]).to_series()

>>> 4500-12-31 00:00:00    4500-12-31 00:00:00
>>> 4600-12-31 00:00:00    4600-12-31 00:00:00
>>> 5100-12-31 00:00:00    5100-12-31 00:00:00
>>> dtype: object

@spencerkclark
Copy link
Member Author

@TomNicholas I'm comforted by the fact that this does not seem to break any existing tests, but does it look OK to you?

@TomNicholas
Copy link
Member

I think this should be fine...

@dcherian
Copy link
Contributor

dcherian commented Dec 7, 2019

test failure is #3603 . Thanks @spencerkclark for the fix, @TomNicholas for reviewing and @mathause for testing!

@dcherian dcherian merged commit 1c446d3 into pydata:master Dec 7, 2019
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
@dcherian
Merge remote-tracking branch 'upstream/master' into dask-median
42a7ed3 
* upstream/master:
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
@dcherian
Merge branch 'master' of github.com:pydata/xarray into chunk-unique-t…
d40a88d 
…oken

* 'master' of github.com:pydata/xarray:
  Add nanmedian for dask arrays (pydata#3604)
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
@dcherian
Merge branch 'master' of github.com:pydata/xarray into optimize-lazy-…
9291b91 
…equiv

* 'master' of github.com:pydata/xarray: (28 commits)
  Add nanmedian for dask arrays (pydata#3604)
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
  Fix map_blocks HLG layering (pydata#3598)
  Silence sphinx warnings: Round 2 (pydata#3592)
  2x~5x speed up for isel() in most cases (pydata#3533)
  remove xarray again (pydata#3591)
  fix plotting with transposed nondim coords. (pydata#3441)
  make coarsen reductions consistent with reductions on other classes (pydata#3500)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

subtracting CFTimeIndex can cause pd.TimedeltaIndex to overflow
4 participants
@spencerkclark @mathause @TomNicholas @dcherian