-
Notifications
You must be signed in to change notification settings - Fork 127
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
Convert QVAnalysis to use BasePlotter #1348
Conversation
This was part of work I did to remove warnings generated by the tests. I think it was somewhat of a mistake to ignore deprecation warnings about plotting rather than updating the plotter when the old plotting methods were deprecated. |
ff0c8b3
to
01cf9fb
Compare
Add an hline method to BaseDrawer and expose linewidth and linestyle as series options. Catch expected warnings about insufficient trials in analysis tests. Remove filters preventing test failures when using the deprecated visualization APIs.
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.
Looks good. The only thing I noticed is that the plotter is limited to one series, which is not the case for other plotters. If it makes sense for quantum volume figures, then I am fine with that limitation; but then we need to document that in the class docstring.
I've added a suggested change to the class docstring to make a note of this limitation. Other than that, it looks good to me. Can be merged once the series limitation/docstring is addressed.
Looks good to me. Thank you for migrating the quantum volume plotting to the new plotting code! |
Add an hline method to BaseDrawer and expose linewidth and linestyle as series options.
Catch expected warnings about insufficient trials in analysis tests.
Remove filters preventing test failures when using the deprecated visualization APIs.