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

Future warning for default reduction dimension of groupby #2366

Merged
merged 14 commits into from
Sep 28, 2018
Merged

Future warning for default reduction dimension of groupby #2366

merged 14 commits into from
Sep 28, 2018

Conversation

fujiisoup
Copy link
Member

@fujiisoup fujiisoup commented Aug 14, 2018 •
edited
Loading

  • Closes #xxxx
  • Tests added
  • Tests passed
  • Fully documented, including whats-new.rst for all changes and api.rst for new API

Started to fix #2363.
Now warns a futurewarning in groupby if default reduction dimension is not specified.
As a side effect, I added xarray.ALL_DIMS. With dim=ALL_DIMS always reduces along all the dimensions.

stickler-ci
stickler-ci reviewed Aug 14, 2018
View reviewed changes
xarray/core/dataarray.py Outdated
@@ -10,7 +10,7 @@
from ..plot.plot import _PlotMethods
from .accessors import DatetimeAccessor
from .alignment import align, reindex_like_indexers
from .common import AbstractArray, DataWithCoords
from .common import AbstractArray, DataWithCoords, ALL_DIMS
Copy link
Contributor

Choose a reason for hiding this comment

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

F401 '.common.ALL_DIMS' imported but unused

xarray/core/groupby.py Outdated
return self.reduce(func, dim, axis, keep_attrs=keep_attrs,
skipna=skipna, allow_lazy=True, **kwargs)
else:
def wrapped_func(self, dim=DEFAULT_DIMS, axis=None, keep_attrs=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

E501 line too long (81 > 79 characters)

@fujiisoup fujiisoup changed the title [WIP] future warning for default reduction dimension of groupby Future warning for default reduction dimension of groupby Aug 16, 2018
@fujiisoup
Copy link
Member Author

I think it is ready for a review.

I am wondering when we can change the default behavior (it only affects groupby/resample for multi-dimensional arrays).
v.0.11? or do we need more time?

shoyer
shoyer reviewed Aug 28, 2018
View reviewed changes
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.

I would hold off for 0.12 on removing this deprecation, since we already very close to 0.11.

xarray/core/common.py Outdated
from .utils import Frozen, SortedKeysDict, ReprObject


# Use as a sentinel value to indicate a all dimensions, which
Copy link
Member

Choose a reason for hiding this comment

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

This sentence looks incomplete

xarray/core/dataset.py Outdated
@@ -2750,6 +2751,8 @@ def reduce(self, func, dim=None, keep_attrs=False, numeric_only=False,
Dataset with this object's DataArrays replaced with new DataArrays
of summarized data and the indicated dimension(s) removed.
"""
if dim == ALL_DIMS:
Copy link
Member

Choose a reason for hiding this comment

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

I usually prefer using is for comparison to sentinel objects -- it looks a little more idiomatic, and is more similar to what we do for comparison to None.

xarray/core/groupby.py Outdated
"warning, pass dim=xarray.ALL_DIMS explicitly.",
FutureWarning, stacklevel=2)
elif dim is None:
dim = self._group_dim
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should add this yet -- this means that code that uses groupby.reduce(... dim=None) would change its behavior. There probably isn't much code like this but I don't see a good reason to change it yet.

@fujiisoup fujiisoup merged commit 638b251 into pydata:master Sep 28, 2018
dcherian pushed a commit to dcherian/xarray that referenced this pull request Oct 7, 2018
Merge branch 'master' into fix/norm
3fb945e 
* master:
  Replace the last of unittest with pytest (pydata#2467)
  Add python_requires to setup.py (pydata#2465)
  Clean up _parse_array_of_cftime_strings (pydata#2464)
  plot.contour: Don't make cmap if colors is a single color. (pydata#2453)
  np.AxisError was added in numpy 1.13 (pydata#2455)
  Add CFTimeIndex.shift (pydata#2431)
  Fix FutureWarning in CFTimeIndex.date_type (pydata#2448)
  fix:2445 (pydata#2446)
  Enable use of cftime.datetime coordinates with differentiate and interp (pydata#2434)
  restore ddof support in std (pydata#2447)
  Future warning for default reduction dimension of groupby (pydata#2366)
  Remove incorrect statement about "drop" in the text docs (pydata#2439)
  Use profile mechanism, not no-op mutation (pydata#2442)
  switch travis language to generic (pydata#2432)
dcherian pushed a commit to maahn/xarray that referenced this pull request Oct 10, 2018
Merge branch 'master' into maahn-groupy_plot2
826df44 
* master: (51 commits)
  xarray.backends refactor (pydata#2261)
  Fix indexing error for data loaded with open_rasterio (pydata#2456)
  Properly support user-provided norm. (pydata#2443)
  pep8speaks (pydata#2462)
  isort (pydata#2469)
  tests shoudn't need to pass for a PR (pydata#2471)
  Replace the last of unittest with pytest (pydata#2467)
  Add python_requires to setup.py (pydata#2465)
  Update whats-new.rst (pydata#2466)
  Clean up _parse_array_of_cftime_strings (pydata#2464)
  plot.contour: Don't make cmap if colors is a single color. (pydata#2453)
  np.AxisError was added in numpy 1.13 (pydata#2455)
  Add CFTimeIndex.shift (pydata#2431)
  Fix FutureWarning in CFTimeIndex.date_type (pydata#2448)
  fix:2445 (pydata#2446)
  Enable use of cftime.datetime coordinates with differentiate and interp (pydata#2434)
  restore ddof support in std (pydata#2447)
  Future warning for default reduction dimension of groupby (pydata#2366)
  Remove incorrect statement about "drop" in the text docs (pydata#2439)
  Use profile mechanism, not no-op mutation (pydata#2442)
  ...
dcherian pushed a commit to dcherian/xarray that referenced this pull request Oct 10, 2018
Merge branch 'master' into 2d-coord
2a2ab82 
* master: (21 commits)
  xarray.backends refactor (pydata#2261)
  Fix indexing error for data loaded with open_rasterio (pydata#2456)
  Properly support user-provided norm. (pydata#2443)
  pep8speaks (pydata#2462)
  isort (pydata#2469)
  tests shoudn't need to pass for a PR (pydata#2471)
  Replace the last of unittest with pytest (pydata#2467)
  Add python_requires to setup.py (pydata#2465)
  Update whats-new.rst (pydata#2466)
  Clean up _parse_array_of_cftime_strings (pydata#2464)
  plot.contour: Don't make cmap if colors is a single color. (pydata#2453)
  np.AxisError was added in numpy 1.13 (pydata#2455)
  Add CFTimeIndex.shift (pydata#2431)
  Fix FutureWarning in CFTimeIndex.date_type (pydata#2448)
  fix:2445 (pydata#2446)
  Enable use of cftime.datetime coordinates with differentiate and interp (pydata#2434)
  restore ddof support in std (pydata#2447)
  Future warning for default reduction dimension of groupby (pydata#2366)
  Remove incorrect statement about "drop" in the text docs (pydata#2439)
  Use profile mechanism, not no-op mutation (pydata#2442)
  ...
@shoyer shoyer mentioned this pull request Sep 4, 2019
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stickler-ci stickler-ci stickler-ci left review comments

@shoyer shoyer shoyer 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.

Reduction APIs for groupby, groupby_bins, resample, rolling
3 participants
@fujiisoup @shoyer @stickler-ci