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

Testing mpl images for graphs #6672

Merged
merged 51 commits into from Jul 14, 2021
Merged

Testing mpl images for graphs #6672

merged 51 commits into from Jul 14, 2021

Conversation

singhmeet11
Copy link
Contributor

Summary

As of now there is no testing module for graph images. This issue will be resolved by this PR and is motivated from issue #6397.

Details and comments

List of changes and additions for creating the tests for graphs.

  1. There was no option for saving graph images like we can save images of circuit using the parameter filename. Made changes in qiskit/visualization/state_visualization.py so that we can save .png using state_drawer . (This has only been implemented for plot_bloch_multivector for now).
  2. Made different folders for Graphs and Circuit in qiskit-terra\test\ipynb\mpl . These house the reference images along with the testing.py files. Made this for clarification and because this resolved an issue I was facing with results.py and mpl_results_graph.py.
  3. mpl_results_graph.py is identical to results.py with a few modifications for the file names.
  4. test_graph_matplotlib_drawer.py works similar to test_circuit_matplotlib_drawer.py. Originally I changed it a bit, but with the modifications to state_visualization.py this works properly.

The test works properly when run via Binder -

new_test

The added test for plot_bloch_multivector can be seen above

Copy link
Contributor

@javabster javabster left a comment

Choose a reason for hiding this comment

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

Thanks for the work so far @singhmeet11! I've left a few comments 😄

qiskit/visualization/state_visualization.py Outdated Show resolved Hide resolved
@@ -0,0 +1,200 @@
# This code is part of Qiskit.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about why a whole new file was created for this? There is a lot of duplicated code here, I think it would be better to try integrate the graph results into the existing mpl_results.py file if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javabster I tried to actually use only one file results.py but because I am creating two files inside the mpl folder( circuit and graph) and the path to specific folders inside these have to be provided in order for the file to work properly, I was not able to integrate the whole code into one file.
How about I remove the folders graph and circuit and store all the data in the same .json file and same references folder(like there won't be any different references and .py files for graph and circuit images). Then only one file will be able to do all the work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can be worked into one file using the specific file names required, I will push a commit shortly to show you what I mean 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I've pushed some changes and it looks like it works now with just the original results.py so this file can be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thanks I was kinda stuck there.

test/ipynb/mpl_tester.ipynb Outdated Show resolved Hide resolved
test/ipynb/results.py Outdated Show resolved Hide resolved
@singhmeet11
Copy link
Contributor Author

singhmeet11 commented Jul 1, 2021

@javabster sorry I forgot to add the link to this PR and I will look into the comments that you have posted in a day or two. Thanks
Plus some of the tests are failing so I also have to look at that

@singhmeet11
Copy link
Contributor Author

@javabster So I have successfully added the tests for paulivec, state_city, Bloch sphere. But I am facing issues with the qsphere, the tests run normally on my laptop but when I run the same tests on binder they fail. The error

ERROR: test_plot_state_qsphere (__main__.TestGraphMatplotlibDrawer)
test for plot_state_qsphere
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/conda/envs/notebook/lib/python3.7/site-packages/qiskit/visualization/state_visualization.py", line 765, in plot_state_qsphere
    import seaborn as sns
ModuleNotFoundError: No module named 'seaborn'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "/home/jovyan/test/ipynb/mpl/graph/test_graph_matplotlib_drawer.py", line 132, in test_plot_state_qsphere
    self.graph_drawer(state=state, output="qsphere", filename="qsphere.png")
  File "/home/jovyan/test/ipynb/mpl/graph/test_graph_matplotlib_drawer.py", line 77, in wrapper
    results = func(*args, **kwargs)
  File "/srv/conda/envs/notebook/lib/python3.7/site-packages/qiskit/visualization/state_visualization.py", line 1304, in state_drawer
    return draw_func(state, **drawer_args)
  File "/srv/conda/envs/notebook/lib/python3.7/site-packages/qiskit/utils/deprecation.py", line 62, in wrapper
    return func(*args, **kwargs)
  File "/srv/conda/envs/notebook/lib/python3.7/site-packages/qiskit/visualization/state_visualization.py", line 771, in plot_state_qsphere
    ) from ex
qiskit.exceptions.MissingOptionalLibraryError: "The 'seaborn' library is required to use 'plot_state_qsphere'. You can install it with 'pip install seaborn'."

So basically the environment that binder is running needs seaborn (I actually used pip to install it and the .ipynb file runs fine) But I don't think adding pip install in the mpl_circuit file will look clean so where do I add the pip install command for seaborn?

The other problem- I also added a test for histogram but after each run, the probabilities change after each run, due to noise. Now, I am looking into how to remove noise completely. I actually tried using the Noise model and equate the probability of error = 0 but that did not work.
Let me know your thoughts on these and thanks for your help.

@singhmeet11
Copy link
Contributor Author

@javabster I have added tests for the 6 main plots, qsphere, paulivec, state_city, histogram, bloch_vector and finally hinton . I don't think there are any other major ones.
One thing the histogram plot is made using counts = {"11": 500, "00": 500} basically I fixed the outcome because executing it leads to fluctuation of counts. Originally thought that this was happening due to noise but https://quantumcomputing.stackexchange.com/questions/9813/how-to-run-the-qiskit-aer-simulator-without-noise shows that the reason for this is different and it is not removable. Moreover I don't think the purpose of this test is to test the simulator and because this will tell if a change has happened in the plot_hisotgram command , I think this is good enough.
Let me know what you think

qiskit_snip
Here is a snapshot of the snapshot tests

@enavarro51
Copy link
Contributor

@singhmeet11 This looks nice. I like the white space separator between the circuit images and the visualizations. And I'd agree you're not trying to catch simulator problems, just the visuals.

@singhmeet11
Copy link
Contributor Author

@javabster The tests run fine too now. Yayy

javabster
javabster previously approved these changes Jul 12, 2021
Copy link
Contributor

@javabster javabster left a comment

Choose a reason for hiding this comment

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

This is great work thanks @singhmeet11! Just one tiny addition, otherwise lgtm 🚀

postBuild Show resolved Hide resolved
Co-authored-by: Abby Mitchell <abby.s.mitchell@gmail.com>
@singhmeet11
Copy link
Contributor Author

singhmeet11 commented Jul 12, 2021

@javabster Thanks! I think this is ready to be merged now. Wait, what does dismissing stale review mean ??

javabster
javabster previously approved these changes Jul 12, 2021
@javabster
Copy link
Contributor

@javabster Thanks! I think this is ready to be merged now. Wait, what does dismissing stale review mean ??

it just means that the commit you pushed mean my previous comment was outdated. Nothing to worry about 😄

@javabster javabster added the Changelog: None Do not include in changelog label Jul 13, 2021
postBuild Outdated Show resolved Hide resolved
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.

This is great! Thanks!

@mergify mergify bot merged commit 6507f98 into Qiskit:main Jul 14, 2021
1ucian0 pushed a commit that referenced this pull request Jul 16, 2021
* Remove excess png and json files from test.ipynb.mpl and graph and move user_style.json to rightful place

* Fix user_style.png
@kdk kdk added this to the 0.19 milestone Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants