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

Adjust tests to use updated pandas syntax for offsets #4537

Merged
merged 9 commits into from
Mar 6, 2021
Merged

Adjust tests to use updated pandas syntax for offsets #4537

merged 9 commits into from
Mar 6, 2021

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Oct 25, 2020 •
edited
Loading

@max-sixty
Copy link
Collaborator Author

For review

dcherian
dcherian reviewed Nov 18, 2020
View reviewed changes
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, @max-sixty

xarray/tests/test_dataarray.py
@@ -2920,9 +2922,11 @@ def test_resample(self):
actual = array.resample(time="24H").reduce(np.mean)
assert_identical(expected, actual)

actual = array.resample(time="24H", loffset="-12H").mean()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is right unless we want to remove the loffset kwarg.

If we keep it, then only expected needs to be changed. We'll need to update the code for loffset in resample.py to avoid the pandas warning.

If we want to remove it, I think we should remove it after indexes are more prominent and user-friendly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you're right.

I was thinking we'd align our API with pandas' — unless we think we should define our own API — but then we actually need to make the changes rather than remove the tests!

keewis
keewis approved these changes Jan 11, 2021
View reviewed changes
Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

I think we can merge this, but we should probably open a new issue to discuss whether we want to deprecate loffset, too.

@max-sixty @keewis
Update xarray/tests/test_dataarray.py
376fb74 
Co-authored-by: keewis <keewis@users.noreply.github.com>
@pep8speaks
Copy link

pep8speaks commented Jan 12, 2021 •
edited
Loading

Hello @max-sixty! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-27 22:02:07 UTC

keewis
keewis reviewed Feb 6, 2021
View reviewed changes
Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

not sure if that fixes it, though.

As for the API, if we don't want to keep the loffset parameter, what about adding a offset or add_offset method to Rolling?

xarray/tests/test_dataset.py Outdated Show resolved Hide resolved
@keewis
Copy link
Collaborator

keewis commented Feb 28, 2021

@max-sixty, now that CI passes: should we merge this?

@dcherian
Copy link
Contributor

dcherian commented Mar 1, 2021

LGTM. Thanks @max-sixty & @keewis

@keewis keewis merged commit dfe50b8 into pydata:master Mar 6, 2021
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 8, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into fix/4107
3f04332 
* upstream/master: (46 commits)
  pin netCDF4=1.5.3 in min-all-deps (pydata#4982)
  fix matplotlib errors for single level discrete colormaps (pydata#4256)
  Adapt exception handling in CFTimeIndex.__sub__ and __rsub__ (pydata#5006)
  Update options.py (pydata#5000)
  Adjust tests to use updated pandas syntax for offsets (pydata#4537)
  add a combine_attrs parameter to Dataset.merge (pydata#4895)
  Support for dask.graph_manipulation (pydata#4965)
  raise on passing axis to Dataset.reduce methods (pydata#4940)
  Whatsnew for 0.17.1 (pydata#4963)
  Refinements to how-to-release (pydata#4964)
  DOC: add example for reindex (pydata#4956)
  DOC: rm np import (pydata#4949)
  Add 0.17.0 release notes (pydata#4953)
  document update as inplace (pydata#4932)
  bump the dependencies (pydata#4942)
  Upstream CI: limit runtime (pydata#4946)
  typing for numpy 1.20 (pydata#4878)
  Use definition of DTypeLike from Numpy if found (pydata#4941)
  autoupdate mypy (pydata#4943)
  Add DataArrayCoarsen.reduce and DatasetCoarsen.reduce methods (pydata#4939)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Mar 18, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into rolling-sliding-w…
517f1f0 
…indow

* upstream/master:
  add polyval to polyfit see also (pydata#5020)
  mention map_blocks in the docstring of apply_ufunc (pydata#5011)
  Switch backend API to v2 (pydata#4989)
  WIP: add new backend api documentation (pydata#4810)
  pin netCDF4=1.5.3 in min-all-deps (pydata#4982)
  fix matplotlib errors for single level discrete colormaps (pydata#4256)
  Adapt exception handling in CFTimeIndex.__sub__ and __rsub__ (pydata#5006)
  Update options.py (pydata#5000)
  Adjust tests to use updated pandas syntax for offsets (pydata#4537)
  add a combine_attrs parameter to Dataset.merge (pydata#4895)
  Support for dask.graph_manipulation (pydata#4965)
  raise on passing axis to Dataset.reduce methods (pydata#4940)
@max-sixty max-sixty deleted the loffset branch April 19, 2021 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keewis keewis keewis approved these changes

@dcherian dcherian dcherian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support operations with pandas Offset objects
4 participants
@max-sixty @pep8speaks @keewis @dcherian