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

Fix compatibility with Matplotlib 3.5 #7301

Merged
merged 3 commits into from
Nov 23, 2021

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Nov 23, 2021

Summary

The problem was qiskit.visualization.bloch.Arrow3D, which subclassed
matplotlib.patches.FancyArrowPatch (a 2D patch), but not the 3D
machinery. It is now made an unholy multiple-inherited abomination of
both the 2D patch and mpl_toolkits.mplot3d.art3d.Patch3D; the latter
is a relatively thin wrapper (in terms of attributes) around the 2D
patch, so this is not too terrible.

Matplotlib 3.5 calls the Patch3D.do_3d_projection method using a
deprecated parameter, triggering two warnings, unless the artist's
module appears to have come from mpl_toolkits.mplot3d.art3d, even if
the new calling convention is respected. The warning is only triggered
when the figure is drawn, which may well be outside of our control, so
we cannot suppress the warnings. Instead, we just lie about the module
the arrow patch was defined in, to trick it into not warning, because we
use the new calling convention. This is supported at least as far back
as Matplotlib 3.3, which is the current minimum supported version. The
nasty hack should be removable once Matplotlib 3.6 is the minimum
version because the deprecation period will expire.

Details and comments

This isn't neat or pretty, but it's an attempt to keep the behaviour exactly as it has been ever since this file came from qutip, since I don't understand mplot3d very well. Fix #7272.

The problem was `qiskit.visualization.bloch.Arrow3D`, which subclassed
`matplotlib.patches.FancyArrowPatch` (a 2D patch), but not the 3D
machinery.  It is now made an unholy multiple-inherited abomination of
both the 2D patch and `mpl_toolkits.mplot3d.art3d.Patch3D`; the latter
is a relatively thin wrapper (in terms of attributes) around the 2D
patch, so this is not too terrible.

Matplotlib 3.5 calls the `Patch3D.do_3d_projection` method using a
deprecated parameter, triggering two warnings, unless the artist's
module appears to have come from `mpl_toolkits.mplot3d.art3d`, even if
the new calling convention is respected.  The warning is only triggered
when the figure is drawn, which may well be outside of our control, so
we cannot suppress the warnings.  Instead, we just lie about the module
the arrow patch was defined in, to trick it into not warning, because we
use the new calling convention.  This is supported at least as far back
as Matplotlib 3.3, which is the current minimum supported version.  The
nasty hack should be removable once Matplotlib 3.6 is the minimum
version because the deprecation period will expire.
@jakelishman jakelishman added priority: high Changelog: Bugfix Include in the "Fixed" section of the changelog labels Nov 23, 2021
@jakelishman jakelishman added this to the 0.19 milestone Nov 23, 2021
@coveralls
Copy link

coveralls commented Nov 23, 2021

Pull Request Test Coverage Report for Build 1496098524

  • 6 of 7 (85.71%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 82.762%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/visualization/bloch.py 6 7 85.71%
Totals Coverage Status
Change from base Build 1495278906: 0.009%
Covered Lines: 50175
Relevant Lines: 60626

💛 - Coveralls

mtreinish
mtreinish previously approved these changes Nov 23, 2021
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, one inline comment typo to fix. I really do think this is worth opening an issue in mpl about (if you haven't already) since this seems like a pretty broken migration/deprecation plan for people. Especially since the Arrow3D code is basically identical in every search result for "3d arrow matplotlib".

Comment on lines +71 to +72
# well, which consequently triggers another deprecation warning. We should be able to remove
# this once 3.6 is the minimum supported version, because the deprecation period ends then.
Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, it'll be a while before we can make our matplotlib requirement >=3.6.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I know - here it's just that I don't like putting in gross hacks without saying the condition that they can be removed again!

qiskit/visualization/bloch.py Outdated Show resolved Hide resolved
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@jakelishman
Copy link
Member Author

Yeah, I'll make the issue on MPL. I didn't before because I didn't know enough about what was going on, but I've read the mplot3d source now and worked out what we were trying to do, so I'm happier that there's no way to faithfully avoid the warning being triggered, and since we're a library and the draw call happens outside our control, we can't apply warning filters without affecting the user's global state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terra incompatible with matplotlib==3.5.0
3 participants