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

Add coarsen.construct #5476

Merged
merged 16 commits into from
Jun 24, 2021
Merged

Add coarsen.construct #5476

merged 16 commits into from
Jun 24, 2021

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Jun 16, 2021

  • Closes Add coarsen.construct #5454
  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

Builds on #5474

Here's an example

import numpy as np
import xarray as xr

ds = xr.Dataset(
    {
        "vart": ("time", np.arange(48)),
        "varx": ("x", np.arange(10)),
        "vartx": (("x", "time"), np.arange(480).reshape(10, 48)),
        "vary": ("y", np.arange(12)),
    },
    coords={"time": np.arange(48), "y": np.arange(12)},
)
ds.coarsen(time=12, x=5, boundary="trim").construct(
    {"time": ("year", "month"), "x": ("x", "x_reshaped")}
)

What do people think of this syntax: {"time": ("year", "month"), "x": ("x", "x_reshaped")? Should we instead do {"time": "month", "x": "x_reshaped"} and have the user later rename the x or time dimension if they want?

@pep8speaks
Copy link

pep8speaks commented Jun 16, 2021

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

Line 16:1: F401 '.test_dataarray.da' imported but unused
Line 17:1: F401 '.test_dataset.ds' imported but unused
Line 20:36: F811 redefinition of unused 'ds' from line 17
Line 27:26: F811 redefinition of unused 'ds' from line 17
Line 42:25: F811 redefinition of unused 'ds' from line 17
Line 58:5: F811 redefinition of unused 'da' from line 16
Line 69:5: F811 redefinition of unused 'da' from line 16
Line 91:5: F811 redefinition of unused 'ds' from line 17
Line 163:5: F811 redefinition of unused 'ds' from line 17
Line 193:25: F811 redefinition of unused 'ds' from line 17
Line 224:5: F811 redefinition of unused 'da' from line 16
Line 276:5: F811 redefinition of unused 'da' from line 16
Line 298:28: F811 redefinition of unused 'da' from line 16
Line 314:5: F811 redefinition of unused 'ds' from line 17

Comment last updated at 2021-06-23 16:18:22 UTC

@dcherian dcherian changed the title Refactor out coarsen tests Add coarsen.construct Jun 16, 2021
Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

Looks good I left some minor comments. I have not looked at the moved tests.

xarray/core/rolling.py Outdated Show resolved Hide resolved
xarray/core/rolling.py Outdated Show resolved Hide resolved
xarray/core/rolling.py Outdated Show resolved Hide resolved
xarray/core/rolling.py Show resolved Hide resolved
xarray/core/rolling.py Outdated Show resolved Hide resolved
xarray/tests/test_coarsen.py Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor Author

This should be ready for review & merge

Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

Looks good.

@dcherian dcherian added plan to merge Final call for comments and removed needs review labels Jun 23, 2021
* main:
  Improve error message for guess engine (pydata#5455)
  Refactor dataset groupby tests (pydata#5506)
  DOC: zarr note on encoding (pydata#5427)
  Allow plotting categorical data (pydata#5464)
@keewis keewis mentioned this pull request Jun 23, 2021
@dcherian
Copy link
Contributor Author

Thanks for the review @mathause

@dcherian dcherian merged commit 8a338ee into pydata:main Jun 24, 2021
@dcherian dcherian deleted the coarsen_reshape branch June 24, 2021 16:55
@github-actions
Copy link
Contributor

Unit Test Results

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 8a338ee. ± Comparison against base commit 8a338ee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add coarsen.construct
3 participants