Added tests for gate_map #2759
Added tests for gate_map #2759
Conversation
drholmie
requested review from
1ucian0,
ajavadia,
atilag,
chriseclectic,
delapuente,
diego-plan9,
ewinston,
jaygambetta,
kdk,
lcapelluto,
mtreinish,
nonhermitian and
taalexander
as code owners
|
|
Fixes Qiskit#2309: qiskit/visualization/gate_map.py needs testing Added two reference images for testing plot_gate_map Added tests for _GraphDist Updated CHANGELOG
There was a problem hiding this comment.
This looks good so far, but rather than using IBMQ it may be better to use the fake backends for this, as they are specifically created for testing purposes. You can see can see an example of how to use them here.
| img_ref = path_to_diagram_reference("5bit_quantum_computer.png") | ||
| for i in self.backend: | ||
| n = i.configuration().n_qubits | ||
| if n > 5: |
There was a problem hiding this comment.
Could you change this to something like if n == 14? This is currently non extensible as if we were to add a device with more qubits this would fail.
There was a problem hiding this comment.
Yup, have made the changes, included tests for 16 and 20 qubit backends as well. Thank you for pointing this out!
Also noticed there was a tiny mistake in the fake backends where faketokyo had n_qubits set to 16 instead of 20, so I changed that to the appropriate value as well.
|
|
||
| def setup(self): | ||
| """ setup for backend """ | ||
| self.backend = IBMQ.backends(filters=lambda x: not x.configuration().simulator) |
There was a problem hiding this comment.
self.backend -> self.backends as it is a list of backends, not a singular device.
There was a problem hiding this comment.
Ah my bad. Have changed this as well.
Thank you again for your feedback and advice on this PR!
| """ tests dist_real calculation for different figsize """ | ||
| params = [(self.ax1, self.real_values[0], True), (self.ax1, self.real_values[1], False), | ||
| (self.ax2, self.real_values[2], True), (self.ax2, self.real_values[3], False)] | ||
| for param1, param2, param3 in params: |
There was a problem hiding this comment.
Could you rename these to make it a bit clearer what each parameter is? (And for the other two tests that also use this structure)
There was a problem hiding this comment.
Ah my bad, have made changes to make parameters clearer.
| for i in self.backends: | ||
| n = i.configuration().n_qubits | ||
| if n == 5: | ||
| img_ref = path_to_diagram_reference("5bit_quantum_computer.png") |
There was a problem hiding this comment.
Could this be changed to
img_ref = path_to_diagram_reference(str(n)+"bit_quantum_computer.png")?
This will be more extensible so in future if more devices are added we won't have to change this code, just add a new reference image.
There was a problem hiding this comment.
Nice! thank you for pointing this out. Made this change as well.
|
|
||
| def test_plot_gate_map(self): | ||
| """ tests plotting of gate map of a device (20 qubit, 16 qubit, 14 qubit and 5 qubit)""" | ||
| for i in self.backends: |
There was a problem hiding this comment.
Could we make this name more descriptive? For example i -> backend
There was a problem hiding this comment.
Alright! have done this too. Thank you once again for reviewing my PR :)
* Added tests for gate_map Fixes Qiskit#2309: qiskit/visualization/gate_map.py needs testing Added two reference images for testing plot_gate_map Added tests for _GraphDist Updated CHANGELOG * Adding PR feedback * Adding PR feedback * Minor fix for comparing images in test_gate_map * Adding PR feedback
Summary
Details and comments