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

change tolerance check on DiagonalGate to match UnitaryGate. #4375

Closed
wants to merge 11 commits into from

Conversation

ewinston
Copy link
Contributor

@ewinston ewinston commented May 1, 2020

Summary

This fixes a bug where it was possible to create a valid UnitaryGate which could be converted to an Isometry but would fail to decompose. The error was due to a difference in tolerances used for validating UnitaryGate and DiagonalGate.

Fixes #4369
Fixes #3842

Details and comments

In the case of UnitaryGate, numpy.allclose with atol=1e-8 and rtol=1e-5 was used while DiagonalGate used it's own implementation with a tolerance of 1e-10. This PR has DiagonalGate use numpy.allclose and references the same tolerances referenced by UnitaryGate.

@ajavadia
Copy link
Member

ajavadia commented May 7, 2020

can you apply this to the new location for the diagonal gate in qiskit.circuit.library? the old location will be deprecated

@ewinston ewinston requested review from chriseclectic, Cryoris and a team as code owners May 20, 2020 18:03
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Is there already a test covering #3842 ?

@ewinston
Copy link
Contributor Author

ewinston commented Jun 8, 2020

@kdk I added a test for #3842.

@Cryoris
Copy link
Contributor

Cryoris commented Jun 14, 2020

Could you add the same tolerance change to circuit.library.generalized_gates.Diagonal? We have an overlap right now with that one and the DiagonalGate class, probably latter should go at some point.

Then I think we can merge this.

@Cryoris Cryoris self-assigned this Jun 16, 2020
@ajavadia
Copy link
Member

Yes can you deprecate the old location. It is leading to confusion.

@Cryoris
Copy link
Contributor

Cryoris commented Jun 18, 2020

Yes can you deprecate the old location. It is leading to confusion.

Yeah I'll open a PR for that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants