-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix backend_util.py and test cases to support Aer 0.13 #11172
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 6901577447
💛 - Coveralls |
Out of curiosity I did a search just to see if either private methods got used elsewhere - seems not - but.... it seems to have been copied elsewhere qiskit/qiskit/visualization/gate_map.py Lines 29 to 31 in e7b1f5e
Maybe as such having some common function somewhere is useful - I'll leave that to the terra-core team to decide. For me its just |
In `test/python/providers/test_fake_backends.py`, ignore running test | ||
when backend is V2. | ||
|
||
In `test/python/pulse/test_block.py` test case is removed because | ||
Aer does not have pulse simulator anymore. |
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.
You can remove these lines as we typically don't need to document test suite changes for our end users as it's not a user facing change.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@@ -226,6 +226,9 @@ def is_simulator_backend(backend): | |||
backend_interface_version = _get_backend_interface_version(backend) | |||
if backend_interface_version <= 1: | |||
return backend.configuration().simulator | |||
else: | |||
if "simulator" in backend.name: |
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.
My concern with this would the that the remote simulator "ibmq_qasm_simulator" would now be treated as local simulator wouldn't it. There is logic in run_circuits that dealt with max circuits around whether things were being run locally or not.
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 think that with the upcoming changes in runtime simulators we shouldn't worry about this too much.
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.
🙈
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.
The PR looks good to me, I just had one question to understand how the controlled gate test relates to the rest of changes in the PR, in case this is something to keep in mind even after backend_utils.py
is removed.
# set number of control qubits | ||
for i in range(num_free_params): | ||
if gate_params[i] == "num_ctrl_qubits": | ||
free_params[i] = 3 |
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 understand that this change is to make the code a bit general and not only work for MCU1Gate
, MCPhaseGate
and MCXGate
, but I was wondering, why did this fail with the new Aer?
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.
The test discovery attempts to discover every registered subclass of ControlledGate
(Python makes that information easily accessible), without restriction to the Qiskit standard library, where it would make sense.
We ideally should probably fix the test discovery to limit it only to controlled gates defined in qiskit.circuit.library.standard_gates
.
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.
So you are suggesting changing the current code on line 1017:
@data(*ControlledGate.__subclasses__())
with something like:
@data([cls for cls in allGates.__dict__.values() if isinstance(cls,type) and issubclass(cls,ControlledGate)])
?
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'm not a huge fan of the automatic discovery in its current form at all, but given the current file, yeah, something like that would be fine. Something a little less magic would be
from qiskit.circuit.library.standard_gates import get_standard_gate_name_mapping
standard_controlled_types = [x.base_class for x in get_standard_gate_name_mapping().values() if isinstance(x, ControlledGate)]
That would exclude the multi-controlled gates, which imo should be handled specially if they're not going to follow the conventions of standard-library gates.
(really, I don't think the mcx etc gates should be in standard_gates
at all, they should be in generalized_gates
, but that's an argument for another time)
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.
Given that it looks like there is a lot of room for improvement in these tests, let's keep the current fix as-is and consider improving the tests in a follow-up.
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, let's merge this PR to further unblock the removal of backend_utils.py
. Thanks a lot @doichanj.
# set number of control qubits | ||
for i in range(num_free_params): | ||
if gate_params[i] == "num_ctrl_qubits": | ||
free_params[i] = 3 |
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.
Given that it looks like there is a lot of room for improvement in these tests, let's keep the current fix as-is and consider improving the tests in a follow-up.
* fix backend_util.py to support Aer 0.13 * Fix test cases for Aer 0.13.0 * format * remove unused imports * Update test/python/providers/test_fake_backends.py Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * fix setting num_control_qubits param in test.python.circuit.test_controlled_gate * remove unused import --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> (cherry picked from commit 45efe66)
* fix backend_util.py to support Aer 0.13 * Fix test cases for Aer 0.13.0 * format * remove unused imports * Update test/python/providers/test_fake_backends.py Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * fix setting num_control_qubits param in test.python.circuit.test_controlled_gate * remove unused import --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> (cherry picked from commit 45efe66) Co-authored-by: Jun Doi <doichan@jp.ibm.com>
* fix backend_util.py to support Aer 0.13 * Fix test cases for Aer 0.13.0 * format * remove unused imports * Update test/python/providers/test_fake_backends.py Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * fix setting num_control_qubits param in test.python.circuit.test_controlled_gate * remove unused import --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
This PR is fix of
qiskit/utils/backend_utils.py
to support Aer 0.13Details and comments
Some test cases with noise model fails with Aer 0.13 release because of
qiskit/qiskit/utils/quantum_instance.py
Line 299 in 5a948c2
is_simulator_backend
always returnsFalse
with BackendV2.This function will be deprecated, but I think we have to add support for V2 to pass test cases.
In
test/python/providers/test_fake_backends.py
, ignore running testwhen backend is V2.
In
test/python/pulse/test_block.py
test case is removed becauseAer does not have pulse simulator anymore.