Standardize visualization signatures and styling. #1359
Conversation
This reverts commit 98e78d0.
nonhermitian
requested review from
1ucian0,
ajavadia,
atilag,
delapuente,
diego-plan9 and
ewinston
as code owners
There was a problem hiding this comment.
I've only made it through the histogram so far, but I found a couple things to fix in there. We probably also need to update the CHANGELOG for everything here, because it will have a lot of user facing implications.
Aside from the inline comments there is also a potential issue with number_to_keep. The docstring says it will make another bar for the other values, but at least in my testing it doesn't do this. It just removes all the values except for the N most frequent. Although in my testing without this branch it looks like what we have in there today doesn't do this either. Maybe we just update the docstring to reflect how it works.
| """Plot a histogram of data. | ||
|
|
||
| Args: | ||
| data (list or dict): This is either a list of dictionaries or a single | ||
| dict containing the values to represent (ex {'001': 130}) | ||
| number_to_keep (int): DEPRECATED the number of terms to plot and rest |
There was a problem hiding this comment.
Heh, well this definitely should be definitely go in the changelog. We deprecated this in 0.6 and now we're un-deprecating it and removing what we said was the alternative.
There was a problem hiding this comment.
Yep, forgot to remove.
|
So I looked into the |
|
if we are going to depreciate plot_state please make sure the tutorial explains all the plotting methods clearly. |
|
@jaygambetta yeah, I'm planning to push an update to the notebook after this merges |
There was a problem hiding this comment.
This LGTM for the most part, I've just got a few inline comments on some small things before we merge this. The biggest things are we're deprecating the multiqubit bloch vector plot without an alternative and the title support for the functions which take a title (which was everything but qsphere) didn't really work all that well. Either too small or put the title on a subplot instead of the whole thing (which looks like an existing bug).
| elif method == "wigner": | ||
| fig = plot_wigner_function(rho, filename=filename, show=show) | ||
| fig = plot_wigner_function(rho) |
There was a problem hiding this comment.
We don't expose this function via qiskit.tools.visualization.__init__ so we'll drop the functionality when we remove plot_state() I'm fine with doing that since this method always felt a bit out of place, and given we need to come to a resolution in #1106 and #1302 I think if we want to make wigner part of the public api we do it as a separate thing.
|
I believe that all known issues are resolved. If this looks good, I will go ahead and update the changelog. |
|
@nonhermitian yeah I think this looks good, so lets update the changelog and then I'll approve. |
This commit updates the tutorial notebook for plotting data with updates to the interface being made in Qiskit/qiskit#1359 . The changes made in that PR will reflect the stable interface of the plotting data functions documented here. So we'll have to update the notebook before the release.
* Update plotting data notebook with interface changes This commit updates the tutorial notebook for plotting data with updates to the interface being made in Qiskit/qiskit#1359 . The changes made in that PR will reflect the stable interface of the plotting data functions documented here. So we'll have to update the notebook before the release. * Finishing Touches * Regenerate to get histogram legend fix
* Fix parallezation in transpile.compile * Revert "Fix parallezation in transpile.compile" This reverts commit 98e78d0. * remove oops files * update plot_histogram * updates * updates * do the imports correctly * fixes * fix style * fix linter * linter again * fix font size * just return None * fix histogram issues * suggested fixes and fixed number_to_keep * update changelog
Summary
An overhaul of the counts and states visualization functions (both mpl and interactive) to standardize the function signatures, styling, and allow for more functionality.
Details and comments
plot_histogram)ImportError: Matploltib not installed.if trying to call the function and mpl is not installed.