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

make coarsen reductions consistent with reductions on other classes #3500

Merged
merged 14 commits into from
Dec 4, 2019

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Nov 8, 2019

  • Tests added
  • Passes black . && mypy . && flake8
  • Fully documented, including whats-new.rst for all changes and api.rst for new API

This PR uses inject_reduce_methods to inject reduction methods into the Coarsen classes. So now we can do coarsen.count() and pass skipna down to the reduction methods.

@max-sixty
Copy link
Collaborator

Great idea, there's lots of room to centralize some of these!

@dcherian
Copy link
Contributor Author

@fujisoup this could use a review if you have time.

Copy link
Member

@fujiisoup fujiisoup left a comment

Choose a reason for hiding this comment

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

Thanks, @dcherian.

It looks very nice!
I don't remember why I added a separate injection function...

Can you add some tests for coarsen.count() and skipna in the reduction methods?

xarray/core/variable.py Outdated Show resolved Hide resolved
dcherian and others added 4 commits November 17, 2019 07:50
* upstream/master: (22 commits)
  Added fill_value for unstack (pydata#3541)
  Add DatasetGroupBy.quantile (pydata#3527)
  ensure rename does not change index type (pydata#3532)
  Leave empty slot when not using accessors
  interpolate_na: Add max_gap support. (pydata#3302)
  units & deprecation merge (pydata#3530)
  Fix set_index when an existing dimension becomes a level (pydata#3520)
  add Variable._replace (pydata#3528)
  Tests for module-level functions with units (pydata#3493)
  Harmonize `FillValue` and `missing_value` during encoding and decoding steps (pydata#3502)
  FUNDING.yml (pydata#3523)
  Allow appending datetime & boolean variables to zarr stores (pydata#3504)
  warn if dim is passed to rolling operations. (pydata#3513)
  Deprecate allow_lazy (pydata#3435)
  Recursive tokenization (pydata#3515)
  format indexing.rst code with black (pydata#3511)
  add missing pint integration tests (pydata#3508)
  DOC: update bottleneck repo url (pydata#3507)
  add drop_sel, drop_vars, map to api.rst (pydata#3506)
  remove syntax warning (pydata#3505)
  ...
…sen-reduce

* 'coarsen-reduce' of github.com:dcherian/xarray:
@dcherian
Copy link
Contributor Author

I don't remember why I added a separate injection function...

👍

Can you add some tests for coarsen.count() and skipna in the reduction methods?

There is a count test in test_variable. I've added sum(skipna=...) tests there too now.

Copy link
Member

@fujiisoup fujiisoup 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 adding the tests. It looks nice!

Probably I wondered if bottleneck works with multiple reduction axes.

doc/whats-new.rst Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor Author

Probably I wondered if bottleneck works with multiple reduction axes.

I'll check this before merging.

@dcherian
Copy link
Contributor Author

dcherian commented Dec 4, 2019

In the case of multiple reduction axes i.e. isinstance(axis, tuple) is True we don't use the bottleneck method so things should be fine.

if (
_USE_BOTTLENECK
and isinstance(values, np.ndarray)
and bn_func is not None
and not isinstance(axis, tuple)
and values.dtype.kind in "uifc"
and values.dtype.isnative
and (dtype is None or np.dtype(dtype) == values.dtype)
):

* upstream/master: (22 commits)
  Resolve the version issues on RTD (pydata#3589)
  Add bottleneck & rasterio git tip to upstream-dev CI (pydata#3585)
  update whats-new.rst (pydata#3581)
  Examples for quantile (pydata#3576)
  add cftime intersphinx entries (pydata#3577)
  Add pyXpcm to Related Projects doc page (pydata#3578)
  Reimplement quantile with apply_ufunc (pydata#3559)
  add environment file for binderized examples (pydata#3568)
  Add drop to api.rst under pending deprecations (pydata#3561)
  replace duplicate method _from_vars_and_coord_names (pydata#3565)
  propagate indexes in to_dataset, from_dataset (pydata#3519)
  Switch examples to notebooks + scipy19 docs improvements (pydata#3557)
  fix whats-new.rst (pydata#3554)
  Tweaks to release instructions (pydata#3555)
  Clarify conda environments for new contributors (pydata#3551)
  Revert to dev version
  0.14.1 whatsnew (pydata#3547)
  sparse option to reindex and unstack (pydata#3542)
  Silence sphinx warnings (pydata#3516)
  Numpy 1.18 support (pydata#3537)
  ...
…sen-reduce

* 'coarsen-reduce' of github.com:dcherian/xarray:
  Update doc/whats-new.rst
@dcherian dcherian merged commit 308bb37 into pydata:master Dec 4, 2019
@dcherian dcherian deleted the coarsen-reduce branch December 4, 2019 16:11
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 5, 2019
* upstream/master: (35 commits)
  fix plotting with transposed nondim coords. (pydata#3441)
  make coarsen reductions consistent with reductions on other classes (pydata#3500)
  Resolve the version issues on RTD (pydata#3589)
  Add bottleneck & rasterio git tip to upstream-dev CI (pydata#3585)
  update whats-new.rst (pydata#3581)
  Examples for quantile (pydata#3576)
  add cftime intersphinx entries (pydata#3577)
  Add pyXpcm to Related Projects doc page (pydata#3578)
  Reimplement quantile with apply_ufunc (pydata#3559)
  add environment file for binderized examples (pydata#3568)
  Add drop to api.rst under pending deprecations (pydata#3561)
  replace duplicate method _from_vars_and_coord_names (pydata#3565)
  propagate indexes in to_dataset, from_dataset (pydata#3519)
  Switch examples to notebooks + scipy19 docs improvements (pydata#3557)
  fix whats-new.rst (pydata#3554)
  Tweaks to release instructions (pydata#3555)
  Clarify conda environments for new contributors (pydata#3551)
  Revert to dev version
  0.14.1 whatsnew (pydata#3547)
  sparse option to reindex and unstack (pydata#3542)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 7, 2019
* upstream/master:
  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)
  Resolve the version issues on RTD (pydata#3589)
  Add bottleneck & rasterio git tip to upstream-dev CI (pydata#3585)
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants