Skip to content

Use unitary equivalence instead of circuit equality in noise tests#1446

Merged
hhorii merged 8 commits into
Qiskit:mainfrom
hitomitak:fix_pauli_test
Mar 4, 2022
Merged

Use unitary equivalence instead of circuit equality in noise tests#1446
hhorii merged 8 commits into
Qiskit:mainfrom
hitomitak:fix_pauli_test

Conversation

@hitomitak
Copy link
Copy Markdown
Contributor

@hitomitak hitomitak commented Feb 4, 2022

Summary

I deleted I gate from the test to reflect this commit.

Details and comments

jakelishman
jakelishman reviewed Feb 4, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This seems right for the immediate test, but the change in Terra was because of a misalignment of expectations around IGate - for historical IBM Q reasons, IGate represents a delay rather than an identity, so it was wrong for opflow to emit them.

I'm slightly concerned in Aer that the whole QuantumError noise model seems to want IGates as input, and depolarising_error emits IGate. I suppose in the context of creating noise models, it's possibly ok?

Here's the docstring about it:
https://github.com/Qiskit/qiskit-terra/blob/226f4646140bd26e0ea70fdfd018a8fd7f156fa8/qiskit/circuit/library/standard_gates/i.py#L20-L24

@mtreinish
Copy link
Copy Markdown
Member

mtreinish commented Feb 4, 2022

We can confirm with @itoko on this but I think that makes sense in this context, at least while the IGate is defined as a single qubit gate delay (which I think is just confusing). It might make sense if we could have things work natively in delay. Eventually we're going to have to do it once we're ready for Qiskit/qiskit#7146 (I'm still not clear how it really breaks that assumption because it translating between I and U(0,0,0) will keep the single gate delay properties) and this will come up again.

@hhorii
Copy link
Copy Markdown
Collaborator

hhorii commented Feb 9, 2022

I believe that, in this case, we can just compare unitary matrices of target and expected circuits, which can include IGate or not.

mtreinish
mtreinish previously approved these changes Mar 1, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread test/terra/noise/test_standard_errors.py Outdated
@hhorii @mtreinish
Use qi.Operator.equiv() instead of equality in a test
080dc2b 
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@jakelishman jakelishman changed the title Delete I gate from a circuit in standard noise test. Use unitary equivalence instead of circuit equality in noise tests Mar 1, 2022
@hhorii hhorii merged commit d8327f0 into Qiskit:main Mar 4, 2022
@hhorii hhorii added the stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable label Apr 4, 2022
hhorii added a commit to hhorii/qiskit-aer that referenced this pull request Apr 4, 2022
@hitomitak @mtreinish @hhorii
Use unitary equivalence instead of circuit equality in noise tests (Q…
29551a6 
…iskit#1446)

* Use unitary equivalence instead of circuit equality in noise tests

Recent qiskit-terra reduces I gates and some tests of Aer failed in equality checks of QuantumCircuit injected as noise.
This PR change a way of equality check from object equality to unitary equivalence of QuantumCircuit.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Hiroshi Horii <horii@jp.ibm.com>
hhorii added a commit to mtreinish/qiskit-aer that referenced this pull request Apr 4, 2022
@hitomitak @mtreinish @hhorii
Use unitary equivalence instead of circuit equality in noise tests (Q…
c8b74a3 
…iskit#1446)

* Use unitary equivalence instead of circuit equality in noise tests

Recent qiskit-terra reduces I gates and some tests of Aer failed in equality checks of QuantumCircuit injected as noise.
This PR change a way of equality check from object equality to unitary equivalence of QuantumCircuit.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Hiroshi Horii <horii@jp.ibm.com>
hhorii added a commit to mtreinish/qiskit-aer that referenced this pull request Apr 4, 2022
@hitomitak @mtreinish @hhorii
Use unitary equivalence instead of circuit equality in noise tests (Q…
a8e6be4 
…iskit#1446)

* Use unitary equivalence instead of circuit equality in noise tests

Recent qiskit-terra reduces I gates and some tests of Aer failed in equality checks of QuantumCircuit injected as noise.
This PR change a way of equality check from object equality to unitary equivalence of QuantumCircuit.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Hiroshi Horii <horii@jp.ibm.com>
@hhorii hhorii mentioned this pull request Apr 4, 2022
Merged
hhorii added a commit that referenced this pull request Apr 4, 2022
@mtreinish @hhorii
Only install terra from binary for wheel jobs (#1499)
d9eb6b9 
In the recently released terra 0.20.0 release the minimum python
packaging spec supported is manylinux2014 now. On the main branch we
bumped the manylinux base image we use for wheel jobs to manylinux2014
in #1498. However, on the stable 0.10.x branch we don't want to do that
since we probably should not drop support for older environments on a
stable release. This commit updates the wheel job config to instead
install terra from a compatible binary wheel instead of using the latest
release. This should hopefully avoid the CI failure but still enable us
to run without building terra from source or dropping support for
manylinux2010.

* Use unitary equivalence instead of circuit equality in noise tests (#1446)

Co-authored-by: Hiroshi Horii <horii@jp.ibm.com>
hhorii added a commit to hhorii/qiskit-aer that referenced this pull request Apr 5, 2022
@mtreinish @hhorii
Only install terra from binary for wheel jobs (Qiskit#1499)
efb32d0 
In the recently released terra 0.20.0 release the minimum python
packaging spec supported is manylinux2014 now. On the main branch we
bumped the manylinux base image we use for wheel jobs to manylinux2014
in Qiskit#1498. However, on the stable 0.10.x branch we don't want to do that
since we probably should not drop support for older environments on a
stable release. This commit updates the wheel job config to instead
install terra from a compatible binary wheel instead of using the latest
release. This should hopefully avoid the CI failure but still enable us
to run without building terra from source or dropping support for
manylinux2010.

* Use unitary equivalence instead of circuit equality in noise tests (Qiskit#1446)

Co-authored-by: Hiroshi Horii <horii@jp.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jakelishman jakelishman jakelishman approved these changes

@mtreinish mtreinish mtreinish left review comments

Assignees

No one assigned

Labels

stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hitomitak @mtreinish @hhorii @jakelishman