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

Convert QVAnalysis to use BasePlotter #1348

Merged
merged 3 commits into from Jan 31, 2024

Conversation

wshanks
Copy link
Contributor

@wshanks wshanks commented Dec 19, 2023

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.

@wshanks
Copy link
Contributor Author

wshanks commented Dec 19, 2023

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.

@wshanks wshanks force-pushed the qvplotter branch 4 times, most recently from ff0c8b3 to 01cf9fb Compare December 20, 2023 18:01
@wshanks wshanks added this to the Release 0.6 milestone Dec 20, 2023
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.
Copy link
Collaborator

@conradhaupt conradhaupt left a 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.

qiskit_experiments/library/quantum_volume/qv_analysis.py Outdated Show resolved Hide resolved
qiskit_experiments/library/quantum_volume/qv_analysis.py Outdated Show resolved Hide resolved
qiskit_experiments/library/quantum_volume/qv_analysis.py Outdated Show resolved Hide resolved
@conradhaupt
Copy link
Collaborator

Looks good to me. Thank you for migrating the quantum volume plotting to the new plotting code!

@conradhaupt conradhaupt added this pull request to the merge queue Jan 31, 2024
Merged via the queue into Qiskit-Extensions:main with commit 32f02b1 Jan 31, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants