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

Fix pint integration tests #3600

Merged
merged 32 commits into from
Dec 9, 2019
Merged

Fix pint integration tests #3600

merged 32 commits into from
Dec 9, 2019

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Dec 5, 2019

This cleans up the tests which became a bit of a mess. Once that is done, we can actively work on fixing #3594.

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

@jthielen jthielen mentioned this pull request Dec 5, 2019
Merged
@jthielen
Copy link
Contributor

jthielen commented Dec 5, 2019

In general, this looks good!

After running through this, a couple thoughts did come to mind:

  • Do you have tests of align/combine_*/concat that have Quantity data but no Quantity coordinates? I think these could help isolate the coordinate/indexing issue from the any other operation-related issue.
  • Would you be able to alter the cumprod tests to test a dimensionless Quantity? np.cumprod is only supported (and only really makes sense) for dimensionless Quantities
  • Could you change the xfail reason for matrix multiplication to pint not supporting einsum? __matmul__ has been implemented in NEP-18 Compatibility hgrecco/pint#905, but xarray's __matmul__ appears to route through np.einsum.
  • Would it make sense to remove the np.rank test as discussed here: NEP-18 Compatibility hgrecco/pint#905 (comment)?

@keewis
Copy link
Collaborator Author

keewis commented Dec 5, 2019

np.rank is removed in one of the yet-to-be-pushed commits.

align/ etc.: not yet, but I can change the variants dict (replace original_unit with 1 if you want to play with it)

cumprod, matmul: sure, will do

@keewis keewis mentioned this pull request Dec 6, 2019
Open
18 tasks
@max-sixty
Copy link
Collaborator

Looks like a good set of improvements, thanks @keewis

Shall we merge this and do flups in another PR?

@keewis
Copy link
Collaborator Author

keewis commented Dec 7, 2019

I'd first like to go through the Dataset tests which probably won't need as much updates as the DataArray tests since I had more experience when I wrote them.

What do you mean with "flups"?

@max-sixty
Copy link
Collaborator

What do you mean with "flups"?

Sorry, that's probably not widely used! "flups"="follow-ups"

@keewis
Copy link
Collaborator Author

keewis commented Dec 8, 2019

I'm done with revisiting the tests so this is ready for review & merge.

@jthielen I think I applied most of the changes you requested. cumprod fails using xarray's structures (works fine with Quantity), so the xfail is still necessary. I also didn't isolate the dims from any tests other than align and I think we still need at least one more iteration to get full isolation (I'd like to leave that to a new PR, though -- maybe when we attempt to silence the UnitStrippedWarnings?).

@jthielen
Copy link
Contributor

jthielen commented Dec 8, 2019

cumprod fails using xarray's structures (works fine with Quantity), so the xfail is still necessary. I also didn't isolate the dims from any tests other than align and I think we still need at least one more iteration to get full isolation (I'd like to leave that to a new PR, though -- maybe when we attempt to silence the UnitStrippedWarnings?).

Sounds like a good plan.

@keewis keewis changed the title WIP: Fix pint integration tests Fix pint integration tests Dec 8, 2019
@max-sixty
Copy link
Collaborator

Looks good! (as ever with these, I had a browse through and picked a couple of sections to look at more deeply, but didn't go through each line in detail)

@max-sixty
Copy link
Collaborator

And test failure is unrelated (and think there's a PR to fix)

@max-sixty max-sixty merged commit 5c674e6 into pydata:master Dec 9, 2019
@keewis
Copy link
Collaborator Author

keewis commented Dec 9, 2019

thanks for reviewing and merging, @max-sixty

@keewis keewis deleted the fix-pint-integration-tests branch December 9, 2019 12:20
@max-sixty
Copy link
Collaborator

Thank you @keewis !

dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
@dcherian
Merge remote-tracking branch 'upstream/master' into dask-median
42a7ed3 
* upstream/master:
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
@dcherian
Merge branch 'master' of github.com:pydata/xarray into chunk-unique-t…
d40a88d 
…oken

* 'master' of github.com:pydata/xarray:
  Add nanmedian for dask arrays (pydata#3604)
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
dcherian added a commit to dcherian/xarray that referenced this pull request Dec 30, 2019
@dcherian
Merge branch 'master' of github.com:pydata/xarray into optimize-lazy-…
9291b91 
…equiv

* 'master' of github.com:pydata/xarray: (28 commits)
  Add nanmedian for dask arrays (pydata#3604)
  added pyinterp to related projects (pydata#3655)
  Allow incomplete hypercubes in combine_by_coords (pydata#3649)
  concat keeps attrs from first variable. (pydata#3637)
  Extend DatetimeAccessor properties and support `.dt` accessor for Timedelta (pydata#3612)
  update readthedocs.yml (pydata#3639)
  silence sphinx warnings round 3 (pydata#3602)
  Fix/quantile wrong errmsg (pydata#3635)
  Provide shape info in shape mismatch error. (pydata#3619)
  Minor doc fixes (pydata#3615)
  Respect user-specified coordinates attribute. (pydata#3487)
  Add Facetgrid.row_labels & Facetgrid.col_labels (pydata#3597)
  Fix pint integration tests (pydata#3600)
  Minor fix to combine_by_coords to allow for the combination of CFTimeIndexes separated by large time intervals (pydata#3543)
  Fix map_blocks HLG layering (pydata#3598)
  Silence sphinx warnings: Round 2 (pydata#3592)
  2x~5x speed up for isel() in most cases (pydata#3533)
  remove xarray again (pydata#3591)
  fix plotting with transposed nondim coords. (pydata#3441)
  make coarsen reductions consistent with reductions on other classes (pydata#3500)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@keewis @jthielen @max-sixty