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

ndrolling repr fix #4329

Merged
merged 4 commits into from
Aug 9, 2020
Merged

ndrolling repr fix #4329

merged 4 commits into from
Aug 9, 2020

Conversation

fujiisoup
Copy link
Member

@fujiisoup fujiisoup commented Aug 8, 2020

There was a bug in rolling.__repr__ but it was not tested.
Fixed and tests are added.

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.

thanks for the quick fix, @fujiisoup. I'd change the test, otherwise this looks good to me.

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

keewis commented Aug 9, 2020

This looks good to me. Unfortunately, now that the repr is fixed, we run into a separate issue, triggered by

xarray/doc/computation.rst

Lines 236 to 237 in 7e1fbf8

# rolling with 2-point stride
rolling_da = r.construct("window_dim", stride=2)

Traceback
ValueError                                Traceback (most recent call last)
<ipython-input-38-e6e456ecd183> in <module>
----> 1 rolling_da = r.construct("window_dim", stride=2)

.../xarray/core/rolling.py in construct(self, window_dim, stride, fill_value, **window_dim_kwargs)
    277             window_dim = {d: window_dim_kwargs[d] for d in self.dim}
    278 
--> 279         window_dim = self._mapping_to_list(
    280             window_dim, allow_default=False, allow_allsame=False
    281         )

.../xarray/core/rolling.py in _mapping_to_list(self, arg, default, allow_default, allow_allsame)
    157             return [arg]
    158         else:
--> 159             raise ValueError("Mapping argument is necessary.")
    160 
    161 

ValueError: Mapping argument is necessary.

Edit: what do you think about using this to fix str-typed window_dims (while it works I'm not sure if that actually makes sense, though):

diff --git a/xarray/core/rolling.py b/xarray/core/rolling.py
index 62d14fad..a5f83250 100644
--- a/xarray/core/rolling.py
+++ b/xarray/core/rolling.py
@@ -275,6 +275,8 @@ class DataArrayRolling(Rolling):
                     "Either window_dim or window_dim_kwargs need to be specified."
                 )
             window_dim = {d: window_dim_kwargs[d] for d in self.dim}
+        elif isinstance(window_dim, str):
+            window_dim = {d: window_dim for d in self.dim}
 
         window_dim = self._mapping_to_list(
             window_dim, allow_default=False, allow_allsame=False

Edit2: that should be the last docs-only failure related to "rolling", I can build the docs locally with that change.

@fujiisoup
Copy link
Member Author

Thanks @keewis for checking.
I'm not sure what causes the error in rolling_da.mean("window_dim", skipna=False)

self._mapping_to_list should handle this problem.
How can I get the details of this error?
I just saw the time-out error...

@keewis
Copy link
Collaborator

keewis commented Aug 9, 2020

normally you'd look at the output of the RTD build, but somehow just the official setup times out instead of printing the log so I put in a support request on RTD.

I can reproduce the failure using:

In [1]: import xarray as xr 
   ...: import numpy as np 
   ...:  
   ...: arr = xr.DataArray(np.arange(0, 7.5, 0.5).reshape(3, 5), dims=("x", "y")) 
   ...: display(arr) 
   ...: r = arr.rolling(x=2, y=3, min_periods=2) 
   ...: display(r) 
   ...:  
   ...: rolling_da = r.construct("window_dim", stride=2) 
   ...: display(rolling_da)
<xarray.DataArray (x: 3, y: 5)>
array([[0. , 0.5, 1. , 1.5, 2. ],
       [2.5, 3. , 3.5, 4. , 4.5],
       [5. , 5.5, 6. , 6.5, 7. ]])
Dimensions without coordinates: x, y
DataArrayRolling [x->2,y->3]
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-1-f9a10c0cfaeb> in <module>
      7 display(r)
      8 
----> 9 rolling_da = r.construct("window_dim", stride=2)
     10 display(rolling_da)
     11 rolling_da.mean("window_dim", skipna=False)

.../xarray/core/rolling.py in construct(self, window_dim, stride, fill_value, **window_dim_kwargs)
    277             window_dim = {d: window_dim_kwargs[d] for d in self.dim}
    278 
--> 279         window_dim = self._mapping_to_list(
    280             window_dim, allow_default=False, allow_allsame=False
    281         )

.../xarray/core/rolling.py in _mapping_to_list(self, arg, default, allow_default, allow_allsame)
    157             return [arg]
    158         else:
--> 159             raise ValueError("Mapping argument is necessary.")
    160 
    161 

ValueError: Mapping argument is necessary.

so the bug is in Rolling.construct.

@fujiisoup
Copy link
Member Author

Thanks, @keewis , for the clarification.

It was a bug in the documentation page but not in rolling.construct.

It should raise an error in this case, because for 2d rolling we need 2 dimension names,

rolling_da = r.construct(x="x_win", y='y_win' , stride=2) 

I corrected the documentation and error message.

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.

Thanks for the quick response, @fujiisoup. Looks good to me. I think this should be ready to go once CI passes.

doc/computation.rst Outdated Show resolved Hide resolved
@keewis
Copy link
Collaborator

keewis commented Aug 9, 2020

thanks, that fixed the build on my own RTD setup, but the main setup is quite a bit slower...

Edit: it passed on the main setup, too. Let's merge on green.

@keewis
Copy link
Collaborator

keewis commented Aug 9, 2020

all CI pass. Thanks for the quick fixes, @fujiisoup

@keewis keewis merged commit df7b2ea into pydata:master Aug 9, 2020
dcherian added a commit to rpgoldman/xarray that referenced this pull request Aug 14, 2020
* 'master' of github.com:pydata/xarray: (260 commits)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  Fix docstring for missing_dims argument to isel methods (pydata#4298)
  Support for PyCharm remote deployment (pydata#4299)
  Update map_blocks and map_overlap docstrings (pydata#4303)
  Lazily load resource files (pydata#4297)
  warn about the removal of the ufuncs (pydata#4268)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 15, 2020
* upstream/master: (34 commits)
  Fix bug in computing means of cftime.datetime arrays (pydata#4344)
  fix some str accessor inconsistencies (pydata#4339)
  pin matplotlib in ci/requirements/doc.yml (pydata#4340)
  Clarify drop_vars return value. (pydata#4244)
  Support explicitly setting a dimension order with to_dataframe() (pydata#4333)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 16, 2020
* upstream/master: (40 commits)
  Fix bug in computing means of cftime.datetime arrays (pydata#4344)
  fix some str accessor inconsistencies (pydata#4339)
  pin matplotlib in ci/requirements/doc.yml (pydata#4340)
  Clarify drop_vars return value. (pydata#4244)
  Support explicitly setting a dimension order with to_dataframe() (pydata#4333)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  ...
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.

failing docs CI
2 participants