Skip to content

Fix example's BasicUnit array conversion.#19535

Merged
tacaswell merged 3 commits intomatplotlib:masterfrom
QuLogic:fix-units-old-np
Mar 25, 2021
Merged

Fix example's BasicUnit array conversion.#19535
tacaswell merged 3 commits intomatplotlib:masterfrom
QuLogic:fix-units-old-np

Conversation

@QuLogic
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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