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

Use .to_numpy() for quantified facetgrids #5886

Merged
merged 10 commits into from
Oct 28, 2021

Conversation

TomNicholas
Copy link
Contributor

@TomNicholas TomNicholas commented Oct 22, 2021

Follows on from #5561 by replacing .values with .to_numpy() in more places in the plotting code. This allows pint.Quantity arrays to be plotted without issuing a UnitStrippedWarning (and will generalise better to other duck arrays later).

I noticed the need for this when trying out this example (but trying it without the .dequantify() call first).

(@Illviljan in theory .values should be replaced with .to_numpy() everywhere in the plotting code by the way)

  • Closes #xxxx
  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

@TomNicholas TomNicholas added topic-plotting topic-arrays related to flexible array support labels Oct 22, 2021
# Pass the data as a masked ndarray too
zval = darray.to_masked_array(copy=False)
zval = zarray.to_masked_array(copy=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite sure what needs to be done here with regards to converting between quantified arrays and masked arrays

Copy link
Contributor Author

@TomNicholas TomNicholas Oct 28, 2021

Choose a reason for hiding this comment

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

Actually I think it's already been done in this previous PR, which means .to_masked_array already dequantifies

https://github.com/pydata/xarray/pull/5561/files#diff-386f35dda14ca12d315f2eb71f0ab35d7fdc73419b5dc5175ea5720f3206903bR2787

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... which means that calling .as_numpy() and then also calling .to_masked_array() unneccessarily repeats calling .values, so I got rid of the former.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 22, 2021

Unit Test Results

         6 files           6 suites   56m 58s ⏱️
16 281 tests 14 545 ✔️ 1 736 💤 0
90 882 runs  82 702 ✔️ 8 180 💤 0

Results for commit baad9de.

♻️ This comment has been updated with latest results.

@TomNicholas TomNicholas mentioned this pull request Oct 23, 2021
@Illviljan
Copy link
Contributor

Did all of these changes print the warning? Would be nice if we could get these tested somehow.

@TomNicholas
Copy link
Contributor Author

Did all of these changes print the warning?

I don't know - instead of testing all the cases I just made the .values->.to_numpy() change everywhere I could see. I think that makes sense given that:
(a) there are tests elsewhere that should guarantee that those two methods are equivalent for non-pint arrays,
(b) making those changes didn't break any existing tests,
(c) .to_numpy() was intended to be the new default for calling within plotting code.

Would be nice if we could get these tested somehow.

I've added some tests for faceted line plots, faceted imshow, and faceted contourf. There is almost certainly some other type of plot or other edge case I haven't tested, but that's enough to show that this PR fixes what it intended to fix.

It would be nicer to know for certain that we are testing all the plot types with pint arrays, but I think that can maybe wait for another PR?

@TomNicholas TomNicholas added the plan to merge Final call for comments label Oct 28, 2021
@Illviljan
Copy link
Contributor

It would be nicer to know for certain that we are testing all the plot types with pint arrays, but I think that can maybe wait for another PR?

Agreed, it can wait for another time. This looks good to me. :)

@pep8speaks
Copy link

pep8speaks commented Oct 28, 2021

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-10-28 22:17:40 UTC

@dcherian
Copy link
Contributor

Thanks @TomNicholas

@dcherian dcherian merged commit 36f05d7 into pydata:main Oct 28, 2021
dcherian added a commit to dcherian/xarray that referenced this pull request Oct 29, 2021
* main:
  Add typing_extensions as a required dependency (pydata#5911)
  pydata#5740 follow up: supress xr.ufunc warnings in tests (pydata#5914)
  Avoid accessing slow .data in unstack (pydata#5906)
  Add wradlib to ecosystem in docs (pydata#5915)
  Use .to_numpy() for quantified facetgrids (pydata#5886)
  [test-upstream] fix pd skipna=None (pydata#5899)
  Add var and std to weighted computations (pydata#5870)
  Check for path-like objects rather than Path type, use os.fspath (pydata#5879)
  Handle single `PathLike` objects in `open_mfdataset()` (pydata#5884)
dcherian added a commit to dcherian/xarray that referenced this pull request Oct 29, 2021
* upstream/main:
  Add typing_extensions as a required dependency (pydata#5911)
  pydata#5740 follow up: supress xr.ufunc warnings in tests (pydata#5914)
  Avoid accessing slow .data in unstack (pydata#5906)
  Add wradlib to ecosystem in docs (pydata#5915)
  Use .to_numpy() for quantified facetgrids (pydata#5886)
  [test-upstream] fix pd skipna=None (pydata#5899)
  Add var and std to weighted computations (pydata#5870)
snowman2 pushed a commit to snowman2/xarray that referenced this pull request Feb 9, 2022
Co-authored-by: Illviljan <14371165+Illviljan@users.noreply.github.com>
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 topic-arrays related to flexible array support topic-plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants