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

Tests for variables with units #3654

Merged
merged 39 commits into from
Jan 15, 2020

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Dec 28, 2019

As promised in #3493, this adds integration tests for units. I'm doing this now rather than later since I encountered a few cases in #3643 where a increased test coverage for variables would have been helpful.

  • Tests added
  • Passes black . && mypy . && flake8

Comment on lines +1266 to +1278
def delete_attrs(*to_delete):
def wrapper(cls):
for item in to_delete:
setattr(cls, item, None)

return cls

return wrapper


@delete_attrs(
"test_getitem_with_mask",
"test_getitem_with_mask_nd_indexer",
Copy link
Collaborator Author

@keewis keewis Dec 28, 2019

Choose a reason for hiding this comment

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

are there better ways to exclude inherited tests? I'd like to avoid marking them as skipped since they will probably never pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea. You could split the parent class into two I guess?

Maybe @max-sixty knows some tricks.

@keewis keewis mentioned this pull request Dec 31, 2019
18 tasks
@keewis
Copy link
Collaborator Author

keewis commented Jan 8, 2020

this should be ready for review and merge.


Also, I found a few issues that I'm posting here so I won't forget about them:

  • documentation:
    • the examples of Variable.rolling_window pass a variable x that is not set anywhere and would change the results
    • the docstring of set_dims is quite confusing (to me, at least), it does not mention the shape parameter (which would be the same as passing a dictionary with values dict(zip(dims, shape)) to dims) or that broadcasting is used to expand if necessary (so no reshaping).
  • code: pad_with_fill_value seems to implement pad for dask arrays for all versions of dask, but at least for recent versions we should rely dask's implementation instead

I can update the documentation, but I'd leave the revisiting of the dask issues (there are a lot of them) to someone else.

@keewis
Copy link
Collaborator Author

keewis commented Jan 9, 2020

the test failures are due to the pint update which means they should not be related. I'll mark them as xfail and fix them in the appropriate PR (#3611 for the apply_ufunc failures and the Dataset PR I will open once all Variable and DataArray tests are passing for the to_stacked_array failures)

@keewis
Copy link
Collaborator Author

keewis commented Jan 9, 2020

this should probably be merged before any of the other related PRs

@keewis keewis changed the title WIP: Tests for variables with units Tests for variables with units Jan 13, 2020
@keewis keewis requested a review from dcherian January 13, 2020 13:40
xarray/tests/test_units.py Show resolved Hide resolved
@@ -28,6 +29,21 @@
]


def is_compatible(unit1, unit2):
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a nice thing to move upstream if pint wants it.

Copy link
Collaborator Author

@keewis keewis Jan 14, 2020

Choose a reason for hiding this comment

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

👍, I'll ask

Edit: see hgrecco/pint#988

xarray/tests/test_units.py Show resolved Hide resolved
xarray/tests/test_units.py Show resolved Hide resolved
Comment on lines +1266 to +1278
def delete_attrs(*to_delete):
def wrapper(cls):
for item in to_delete:
setattr(cls, item, None)

return cls

return wrapper


@delete_attrs(
"test_getitem_with_mask",
"test_getitem_with_mask_nd_indexer",
Copy link
Contributor

Choose a reason for hiding this comment

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

No idea. You could split the parent class into two I guess?

Maybe @max-sixty knows some tricks.

xarray/tests/test_units.py Outdated Show resolved Hide resolved
xarray/tests/test_units.py Show resolved Hide resolved

assert expected == actual

@pytest.mark.parametrize(
Copy link
Contributor

Choose a reason for hiding this comment

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

I stopped here. Will look at the rest tomorrow.

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.

just some minor comments. Feel free to merge when you're ready.

Thanks @keewis. It's impressive that so much of this works!

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

keewis commented Jan 15, 2020

I'll merge once the tests pass.

@keewis keewis merged commit 3955c37 into pydata:master Jan 15, 2020
@keewis keewis deleted the tests-for-variables-with-units branch January 15, 2020 16:53
@keewis
Copy link
Collaborator Author

keewis commented Jan 15, 2020

thanks for the review, @dcherian

dcherian added a commit to dcherian/xarray that referenced this pull request Jan 20, 2020
…e-meta

* 'master' of github.com:pydata/xarray:
  Feature/align in dot (pydata#3699)
  ENH: enable `H5NetCDFStore` to work with already open h5netcdf.File a… (pydata#3618)
  One-off isort run (pydata#3705)
  hardcoded xarray.__all__ (pydata#3703)
  Bump mypy to v0.761 (pydata#3704)
  remove DataArray and Dataset constructor deprecations for 0.15  (pydata#3560)
  Tests for variables with units (pydata#3654)
dcherian added a commit to dcherian/xarray that referenced this pull request Jan 21, 2020
* upstream/master: (23 commits)
  Feature/align in dot (pydata#3699)
  ENH: enable `H5NetCDFStore` to work with already open h5netcdf.File a… (pydata#3618)
  One-off isort run (pydata#3705)
  hardcoded xarray.__all__ (pydata#3703)
  Bump mypy to v0.761 (pydata#3704)
  remove DataArray and Dataset constructor deprecations for 0.15  (pydata#3560)
  Tests for variables with units (pydata#3654)
  Add an example notebook using apply_ufunc to vectorize 1D functions (pydata#3629)
  Use encoding['dtype'] over data.dtype when possible within CFMaskCoder.encode (pydata#3652)
  allow passing any iterable to drop when dropping variables (pydata#3693)
  Typo on DataSet/DataArray.to_dict documentation (pydata#3692)
  Fix mypy type checking tests failure in ds.merge (pydata#3690)
  Explicitly convert result of pd.to_datetime to a timezone-naive type (pydata#3688)
  ds.merge(da) bugfix (pydata#3677)
  fix docstring for combine_first: returns a Dataset (pydata#3683)
  Add option to choose mfdataset attributes source. (pydata#3498)
  How do I add a new variable to dataset. (pydata#3679)
  Add map_blocks example to whats-new (pydata#3682)
  Make dask names change when chunking Variables by different amounts. (pydata#3584)
  raise an error when renaming dimensions to existing names (pydata#3645)
  ...
dcherian added a commit to fujiisoup/xarray that referenced this pull request Jan 25, 2020
* 'master' of github.com:pydata/xarray: (27 commits)
  bump min deps for 0.15 (pydata#3713)
  setuptools-scm and isort tweaks (pydata#3720)
  Allow binned coordinates on 1D plots y-axis. (pydata#3685)
  apply_ufunc: Add meta kwarg + bump dask to 2.2 (pydata#3660)
  setuptools-scm and one-liner setup.py (pydata#3714)
  Feature/align in dot (pydata#3699)
  ENH: enable `H5NetCDFStore` to work with already open h5netcdf.File a… (pydata#3618)
  One-off isort run (pydata#3705)
  hardcoded xarray.__all__ (pydata#3703)
  Bump mypy to v0.761 (pydata#3704)
  remove DataArray and Dataset constructor deprecations for 0.15  (pydata#3560)
  Tests for variables with units (pydata#3654)
  Add an example notebook using apply_ufunc to vectorize 1D functions (pydata#3629)
  Use encoding['dtype'] over data.dtype when possible within CFMaskCoder.encode (pydata#3652)
  allow passing any iterable to drop when dropping variables (pydata#3693)
  Typo on DataSet/DataArray.to_dict documentation (pydata#3692)
  Fix mypy type checking tests failure in ds.merge (pydata#3690)
  Explicitly convert result of pd.to_datetime to a timezone-naive type (pydata#3688)
  ds.merge(da) bugfix (pydata#3677)
  fix docstring for combine_first: returns a Dataset (pydata#3683)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Jan 27, 2020
* master:
  Add support for CFTimeIndex in get_clean_interp_index (pydata#3631)
  sel with categorical index (pydata#3670)
  bump min deps for 0.15 (pydata#3713)
  setuptools-scm and isort tweaks (pydata#3720)
  Allow binned coordinates on 1D plots y-axis. (pydata#3685)
  apply_ufunc: Add meta kwarg + bump dask to 2.2 (pydata#3660)
  setuptools-scm and one-liner setup.py (pydata#3714)
  Feature/align in dot (pydata#3699)
  ENH: enable `H5NetCDFStore` to work with already open h5netcdf.File a… (pydata#3618)
  One-off isort run (pydata#3705)
  hardcoded xarray.__all__ (pydata#3703)
  Bump mypy to v0.761 (pydata#3704)
  remove DataArray and Dataset constructor deprecations for 0.15  (pydata#3560)
  Tests for variables with units (pydata#3654)
  Add an example notebook using apply_ufunc to vectorize 1D functions (pydata#3629)
  Use encoding['dtype'] over data.dtype when possible within CFMaskCoder.encode (pydata#3652)
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.

2 participants