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

Pint support for top-level functions #3611

Merged
merged 27 commits into from
Mar 9, 2020
Merged

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Dec 11, 2019

This PR tries to get the pint integration tests (see #3594) to pass. To make sure this does not become a giant PR, it is limited to the top-level functions and the work on the tests for DataArray and Dataset will be done in other PRs.

I added pint master to the upstream-dev CI since that way CI finally helps finding bugs. Should I have edited py36-min-nep18 instead?

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

Also, I finally ran into a situation where I have to use assert_allclose instead of assert_equal_with_units. However, assert_allclose does not check for identical units, I think? Would it make sense to copy the code of assert_allclose and add the strict unit checking or would it be better to just use assert_allclose and something like assert extract_units(expected) == extract_units(actual)?

Failing tests list from #3594:

  • align: needs more investigation partially blocked by where, IndexVariable and a bug in assert_equal_with_units and / or the tests, also a behaviour decision on align works (but units in IndexVariable still fail)
  • combine_by_coords: needs more investigation partially blocked by IndexVariable and a test bug (due to assign_coords with mixed DataArray / array args removes coords #3483) works (but units in IndexVariable still fail)
  • combine_nested: needs more investigation blocked by reindex works (but units in IndexVariable still fail)
  • concat: needs more investigation works (but units in IndexVariable still fail)
  • merge: needs more investigation works (but units in IndexVariable still fail)
  • full_like: pint currently does not implement copyto blocked by Expected behavior of np.*_like functions hgrecco/pint#882
  • where: needs more investigation unit is stripped because pint does not implement astype and test bug in test_where_dataset works
  • dot: pint currently does not implement einsum works

@keewis
Copy link
Collaborator Author

keewis commented Dec 11, 2019

it seems the changes to Variable._getitem_with_mask will make other tests fail. The reason for the changes is that pint will convert every argument of np.where to the first argument's units, which would convert the array to the fill value's units and would be quite unexpected, I think?. Does anyone know what to do here?

@keewis
Copy link
Collaborator Author

keewis commented Dec 12, 2019

the _getitem_with_mask failures are due to bool not defining __invert__, which means it falls back to int.__invert__. I special-cased it, but we could also modify indexing.create_mask to always return a duck array.

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

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

the failing matrix multiply tests should be fixed by hgrecco/pint#926.

I think this is ready for review and merge?

xarray/tests/test_units.py Outdated Show resolved Hide resolved
xarray/tests/test_units.py Outdated Show resolved Hide resolved
xarray/tests/test_units.py Outdated Show resolved Hide resolved
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.

thanks @keewis

Should I have edited py36-min-nep18 instead?Should I have edited py36-min-nep18 instead?

Let's do this once there's a pint release.

ci/azure/install.yml Outdated Show resolved Hide resolved
xarray/tests/test_units.py Outdated Show resolved Hide resolved
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
xarray/tests/test_units.py Outdated Show resolved Hide resolved
xarray/tests/test_units.py Show resolved Hide resolved
xarray/core/variable.py Outdated Show resolved Hide resolved
@max-sixty
Copy link
Collaborator

Thanks for the review @dcherian !

@keewis FYI I am starting a new job so will be lying low on xarray for a couple of months, forgive the temporary pause in speedy reviews. It was great to work together for the past few months!

The whole team is looking forward to working more closely with you in the future. We'd like to invite you to be a maintainer: could you drop me an email at m at maxroos period com?

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

keewis commented Dec 19, 2019

I added a whats-new.rst entry, but I'm not sure if it is too minimal.

If this is okay for now, we could merge?

@keewis keewis mentioned this pull request Jan 9, 2020
2 tasks
@dcherian dcherian mentioned this pull request Jan 17, 2020
11 tasks
@keewis keewis mentioned this pull request Jan 17, 2020
3 tasks
@keewis keewis changed the title Pint support for top-level functions WIP: Pint support for top-level functions Jan 17, 2020
@keewis
Copy link
Collaborator Author

keewis commented Mar 7, 2020

This should be ready for review / merge once again.

It seems that after the merge of #3706 all that was left to get the top-level function tests to pass was to clean up those tests.

@keewis keewis changed the title WIP: Pint support for top-level functions Pint support for top-level functions Mar 7, 2020
@max-sixty
Copy link
Collaborator

Looks good!
(as ever with me & this section of code, without being that confident)

@dcherian
Copy link
Contributor

dcherian commented Mar 9, 2020

Yeah, thanks @keewis

@dcherian dcherian merged commit 9f97c43 into pydata:master Mar 9, 2020
@keewis keewis deleted the pint-support branch March 9, 2020 11:35
dcherian added a commit to ej81/xarray that referenced this pull request Mar 14, 2020
…e-size

* upstream/master: (24 commits)
  Fix alignment with join="override" when some dims are unindexed (pydata#3839)
  Fix CFTimeIndex-related errors stemming from updates in pandas (pydata#3764)
  Doctests fixes (pydata#3846)
  add xpublish to related projects (pydata#3850)
  update installation instruction (pydata#3849)
  Pint support for top-level functions (pydata#3611)
  un-xfail tests that append to netCDF files with scipy (pydata#3805)
  remove panel conversion (pydata#3845)
  Add nxarray to related-projects.rst (pydata#3848)
  Implement skipna kwarg in xr.quantile (pydata#3844)
  Allow `where` to receive a callable (pydata#3827)
  update macos image (pydata#3838)
  Label "Installed Versions" item in Issue template (pydata#3832)
  Add note on diff's n differing from pandas (pydata#3822)
  DOC: Add rioxarray and other external examples (pydata#3757)
  Use stable RTD image.
  removed mention that 'dims' are inferred from 'coords'-dict when omit… (pydata#3821)
  Coarsen keep attrs 3376 (pydata#3801)
  Turn on html repr by default (pydata#3812)
  Fix zarr append with groups (pydata#3610)
  ...
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.

3 participants