Skip to content

Fix example's BasicUnit array conversion. #19535

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
Merged
tacaswell merged 3 commits into from
Mar 25, 2021
Merged

Fix example's BasicUnit array conversion. #19535

tacaswell merged 3 commits into from
Mar 25, 2021

Conversation

@QuLogic
Copy link
Member

@QuLogic QuLogic commented Feb 18, 2021 •
edited
Loading

PR Summary

The unit examples are broken in NumPy < 1.20. This fixes a bug that was exposed by #19415 , but does not fix the examples fully. radian_demo is fixed by this change, but ellipse_with_units is still broken. I do not understand the NumPy subclassing methods enough to fix it short of reverting #19415..

A unit is a scalar, not a length-1 array. Though BasicUnit implements __rmul__, if multiplying by an array, the NumPy implementation will call __array__ instead. If the LHS is an array, everything is fine, but if the LHS is a scalar, the previous code would incorrectly cause it to be upcast to a 1D array. When __getitem__ was added in #19415, np.atleast_1d started iterating each (now 1D, not scalar) TaggedValue, seeing it was length 1, and made the x/y arrays into (N, 1) instead of (N,).

PR Checklist

  • [n/a] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [n/a] New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@QuLogic QuLogic added this to the v3.4.0 milestone Feb 18, 2021
@QuLogic
Copy link
Member Author

QuLogic commented Feb 18, 2021 •
edited
Loading

The remaining failure is:

$python examples/units/ellipse_with_units.py 
Traceback (most recent call last):
  File "examples/units/ellipse_with_units.py", line 34, in <module>
    x += xcenter
TypeError: unsupported operand type(s) for +: 'TaggedValue' and 'float'

I am using NumPy 1.18.1/1.18.5. It appears to have fixed itself in 1.19.0. @dstansby

@tacaswell
Copy link
Member

tacaswell commented Feb 19, 2021 •
edited
Loading

It seems that our attempts to use a minimum version of numpy is getting foiled on cricle and we are running the minimum version docs build with np 1.20 🤦 .

@QuLogic
Copy link
Member Author

QuLogic commented Feb 19, 2021

Good catch; hopefully this should work to fix that.

QuLogic added 3 commits March 25, 2021 04:16
@QuLogic
Fix example's BasicUnit array conversion.
2078daa 
A unit is a scalar, not a length-1 array. Though `BasicUnit` implements
`__rmul__`, if multiplying by an array, the NumPy implementation will
call `__array__` instead. If the LHS is an array, everything is fine,
but if the LHS is a scalar, the previous code would incorrectly cause it
to be upcast to a 1D array. When `__getitem__` was added in matplotlib#19415,
`np.atleast_1d` started iterating each (now 1D, not scalar)
`TaggedValue`, seeing it was length 1, and made the x/y arrays into
(N, 1) instead of (N,).
@QuLogic
ci: Install Python dependencies in one go on Circle.
9038b45
@QuLogic
DOC: Only define __getitem__ on NumPy 1.20+.
3488dc7 
On older versions (1.18), this breaks things, so must be skipped.
@jklymak
Copy link
Member

jklymak commented Mar 25, 2021

I looked to see what this example does, but it doesn't do anything except register itself?

@tacaswell tacaswell merged commit 1fdc2cd into matplotlib:master Mar 25, 2021
@lumberbot-app
Copy link

lumberbot-app bot commented Mar 25, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v3.4.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 1fdc2cda42f642a09dea727ba058efa75dd5cc2e
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am "Backport PR #19535: Fix example's BasicUnit array conversion."
  1. Push to a named branch :
git push YOURFORK v3.4.x:auto-backport-of-pr-19535-on-v3.4.x
  1. Create a PR against branch v3.4.x, I would have named this PR:

"Backport PR #19535 on branch v3.4.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@QuLogic QuLogic deleted the fix-units-old-np branch March 25, 2021 19:31
@QuLogic
Copy link
Member Author

QuLogic commented Mar 25, 2021

I looked to see what this example does, but it doesn't do anything except register itself?

It's imported by all the other examples in this directory.

@QuLogic
Copy link
Member Author

QuLogic commented Mar 25, 2021

With other backports done, I hope this will work now.

@meeseeksdev backport to v3.4.x

@meeseeksmachine meeseeksmachine mentioned this pull request Mar 25, 2021
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 25, 2021
@tacaswell @meeseeksmachine
Backport PR matplotlib#19535: Fix example's BasicUnit array conversion.
b46c7c5
QuLogic added a commit that referenced this pull request Mar 25, 2021
@QuLogic
Merge pull request #19772 from meeseeksmachine/auto-backport-of-pr-19…
bdb7774 
…535-on-v3.4.x

Backport PR #19535 on branch v3.4.x (Fix example's BasicUnit array conversion.)
@QuLogic QuLogic mentioned this pull request Apr 1, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes

Assignees

No one assigned

Labels

Documentation topic: units and array ducktypes

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@QuLogic @tacaswell @jklymak