-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Change plot methods according to recommendations in plotting.md
#2286
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
Conversation
| self._gnd_state_probs)] | ||
|
|
||
| def plot(self, **plot_kwargs: Any) -> None: | ||
| def plot(self, ax: plt.Axes, **plot_kwargs: Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument should be optional, and there should be a show=False parameter to force the show.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not what I understood from #2097 or my recommendation on best practices. This is a library method. It should never call show. And the ax should be mandatory to avoid side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a convenience method for users. They should not have to perform boilerplate in order to see the plot. We should support using it with the boilerplate in order to create more complicated plots, but the most important function of this method is "I want to see a plot". Our job is to make that easy to do, instead of requiring the user to look up pyplot documentation in order to understand what they have to pass in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly the point of a global cirq.plot function that I introduced in #2097, which is a convenience function that works with all classes that has a plot(ax, **plot_args) method (OK I called it _plot_ method in #2097, but it might as well be plot method if people like it more).
I view the plot() method as a graphical version of __str__() method, and cirq.plot() as a graphical version of print().
There is not much difference between
obj.plot()and
cirq.plot(obj)in terms of convenience. But the latter allows obj.plot() to be a library that can be used in both other libraries and interactive sessions seamlessly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a global plot method can have enough scope into the specific plotting method in order to do a good job. For example, some plot methods may allow you to control the scale of some widget. The auto-complete help you get from pycharm will not help you discover this if we use a global utility method. This large variety makes it difficult to create a good global method.
I also think that a global plot method is setting up expectations that most objects in cirq will be plottable. Like I should be able to plot gates and wavefunctions and circuits and etc. That would be cool, but is quite a lot of work.
Finally, the amount of work handled by the global plot method is not that large. It's roughly akin to random methods taking an optional generator, and falling back to the np.random module otherwise. It's a bit annoying, but not really that much code in an absolute sense.
For those three reasons (function diversity, implication of generalization, and low repetition cost) I don't think it should be a global utility.
So, basically, make obj.plot() work with some duplicated functionality between the various result objects.
Ideally users using ipython or invoking python from the command line or from pycharm or from jupyter or from collab should all see a plot pop up if they call obj.plot(). My preference would be to do that without a show argument, but people seem to believe that's not possible and that we either need to have warnings in jupyter or no functionality from the command line. I keep alternating between "a warning is better than not having anything happen" and "it would be best if it's at least possible to get perfect functioning in notebook examples".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent #2304 for review. It captures the requirements and recommended practices for plot methods. Once we agree with the recommendation doc, I'll modify codes in this PR accordingly.
|
I've updated the I'll update the |
Strilanc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
You may also want to update scatter_plot_normalized_kak_interaction_coefficients
plot methods according to recommendations in plotting.md
According to the conclusion in #2097.
One remaining class is
TomographyResult, where theplot()method creates 2 axes and plots on them, so a single ax arg doesn't work. It will be addressed once a conclusion is reached on how the signature should be (aFigure, 2Axes, or something else).