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

Add ax kwarg to matplotlib visualization functions #3053

Merged
merged 9 commits into from
Oct 8, 2019

Conversation

mtreinish
Copy link
Member

Summary

This commit adds support for passing a matplotlib axes to visualization
functions. This enables integrating the qiskit visualizations into a
larger matplotlib visualization. When an ax kwarg is set that input will
be used for the output visualization and a new figure will not be
generated or returned.

Details and comments

Fixes #1950

@1ucian0
Copy link
Member

1ucian0 commented Sep 10, 2019

Given that matplotlib calls the module axes, why not calling the argument axes?

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

It seems that the parameters are not optional. Previously working code is breaking :

psi = [0.70710678118654746, 0, 0, 0.70710678118654746]
plot_state_hinton(psi)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-7e1883ce9c63> in <module>
      1 psi = [0.70710678118654746, 0, 0, 0.70710678118654746]
----> 2 plot_state_hinton(psi)

~/repos/qiskit-terra/qiskit/visualization/state_visualization.py in plot_state_hinton(rho, title, figsize, ax_real, ax_imag)
     99             fig = ax_real.get_figure()
    100         else:
--> 101             fig = ax_imag.get_figure()
    102         ax1 = ax_real
    103         ax2 = ax_imag

AttributeError: 'NoneType' object has no attribute 'get_figure'

It's not obvious for me how this new options are used. Maybe a more explicit subplot example in the release notes?

@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Sep 24, 2019
@mtreinish
Copy link
Member Author

It seems that the parameters are not optional. Previously working code is breaking :

psi = [0.70710678118654746, 0, 0, 0.70710678118654746]
plot_state_hinton(psi)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-7e1883ce9c63> in <module>
      1 psi = [0.70710678118654746, 0, 0, 0.70710678118654746]
----> 2 plot_state_hinton(psi)

~/repos/qiskit-terra/qiskit/visualization/state_visualization.py in plot_state_hinton(rho, title, figsize, ax_real, ax_imag)
     99             fig = ax_real.get_figure()
    100         else:
--> 101             fig = ax_imag.get_figure()
    102         ax1 = ax_real
    103         ax2 = ax_imag

AttributeError: 'NoneType' object has no attribute 'get_figure'

It's not obvious for me how this new options are used. Maybe a more explicit subplot example in the release notes?

It was a typo in plot_state_hinton() fixed in the latest patch. I'll take another look at release note and see if I can come up with a clearer example.

@1ucian0
Copy link
Member

1ucian0 commented Oct 2, 2019

do you have any thoughts about #3053 (comment) ?

@mtreinish
Copy link
Member Author

do you have any thoughts about #3053 (comment) ?

Sorry I missed that among the other review comments earlier. I really don't care about the name much, ax is also commonly used in the matplotlib documentation for things that deal with an Axes object. I'd be fine deferring to whatever your preference is, however in this case we are already using ax in an existing API, see: https://github.com/Qiskit/qiskit-terra/blob/master/qiskit/visualization/gate_map.py#L70-L80 so I used ax to be consistent.

.pylintrc Show resolved Hide resolved
@1ucian0
Copy link
Member

1ucian0 commented Oct 3, 2019

however in this case we are already using ax in an existing API

Okey. That's a good argument to keep it. Let's merge.

@mtreinish
Copy link
Member Author

@1ucian0 I manually rebased the branch locally to correct the broken merge that was accidentally pushed removing the pylint disable. You should double check that I didn't mess something up in that process because there were a lot of changes in that rebase stack and I might have messed up one of the conflict fixes.

mtreinish and others added 5 commits October 7, 2019 15:04
This commit adds support for passing a matplotlib axes to visualization
functions. This enables integrating the qiskit visualizations into a
larger matplotlib visualization. When an ax kwarg is set that input will
be used for the output visualization and a new figure will not be
generated or returned.

Fixes Qiskit#1950
This commit adds support for passing a matplotlib axes to visualization
functions. This enables integrating the qiskit visualizations into a
larger matplotlib visualization. When an ax kwarg is set that input will
be used for the output visualization and a new figure will not be
generated or returned.

Fixes Qiskit#1950
As part of rebasing and cleaning up the branch from several failed merge
attempts a few lines from plot_state_city() were lost. This commit fixes
that mistake and restores the z axis labels which were missing from the
imaginary component subplot.
@1ucian0 1ucian0 merged commit 352f940 into Qiskit:master Oct 8, 2019
@mtreinish mtreinish deleted the support-ax-in-mpl branch October 8, 2019 12:55
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
* Add ax kwarg to matplotlib visualization functions

This commit adds support for passing a matplotlib axes to visualization
functions. This enables integrating the qiskit visualizations into a
larger matplotlib visualization. When an ax kwarg is set that input will
be used for the output visualization and a new figure will not be
generated or returned.

Fixes Qiskit#1950

* typo in release note

* Add ax kwarg to matplotlib visualization functions

This commit adds support for passing a matplotlib axes to visualization
functions. This enables integrating the qiskit visualizations into a
larger matplotlib visualization. When an ax kwarg is set that input will
be used for the output visualization and a new figure will not be
generated or returned.

Fixes Qiskit#1950

* Fix lint and typos

* pylint: disable=invalid-name

* Fix lost lines from rebase

As part of rebasing and cleaning up the branch from several failed merge
attempts a few lines from plot_state_city() were lost. This commit fixes
that mistake and restores the z axis labels which were missing from the
imaginary component subplot.

* Fix rebase typo

* Revert "pylint: disable=invalid-name"

This reverts commit 5566611.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visualizations should allow for passing a Matplotlib axes instance
2 participants