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.

Sign up for GitHub

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

Jump to bottom

Approximately equal circuits with symbols not detected by cirq.approx_eq #3195

Merged
merged 11 commits into from
Aug 10, 2020
Merged

Approximately equal circuits with symbols not detected by cirq.approx_eq #3195

merged 11 commits into from
Aug 10, 2020

Conversation

tonybruguier-google
Copy link
Contributor

@tonybruguier-google tonybruguier-google commented Aug 6, 2020 •
edited by balopat

Approximately equal circuits with symbols not detected by cirq.approx_eq (Fixes #3192.)

We should use sympy to make comparisons when available.

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Aug 6, 2020
@tonybruguier-google tonybruguier-google mentioned this pull request Aug 6, 2020
Closed
balopat
balopat suggested changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

This looks great, thank you! I added two minor nits.
Can you also change in the description of the PR ISSUE-3192 to "Fixes #3192." ? That will automatically close the issue when closing the PR!

cirq/protocols/approximate_equality_protocol.py Outdated
@@ -94,6 +95,11 @@ def approx_eq(val: Any, other: Any, *, atol: Union[int, float] = 1e-8) -> bool:
if isinstance(val, str):
return val == other

if isinstance(val, sympy.core.mul.Mul) or isinstance(
other, sympy.core.mul.Mul):
delta = sympy.simplify(sympy.Abs(other - val))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the simplify here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't do that, the unit test fails. Upon inspection, if I print delta, I see delta=Abs((s + 1.51)/pi - (s + 1.515)/pi)

Copy link
Contributor

@balopat balopat Aug 6, 2020 •
edited

Choose a reason for hiding this comment

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

Got it - however, this is still not kosher. There might be a case where you can't evaluate to a number literal and then this question is undecidable. We should handle that and throw an error instead of silently returning False.

sympy.Abs(val - other).simplify().is_number should give you a good indication whether the simplification worked.

Please add a test case covering that too.

cirq/protocols/approximate_equality_protocol_test.py Outdated Show resolved Hide resolved
@tonybruguier-google
Copy link
Contributor Author

Thanks. PTAL.

@tonybruguier-google tonybruguier-google changed the title ISSUE-3192 Approximately equal circuits with symbols not detected by cirq.approx_eq Fixes #3192. Aug 6, 2020
@balopat balopat changed the title Fixes #3192. Approximately equal circuits with symbols not detected by cirq.approx_eq Aug 6, 2020
balopat
balopat suggested changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

I dug a bit deeper, added some more comments! Thank you!

cirq/protocols/approximate_equality_protocol.py Outdated Show resolved Hide resolved
cirq/protocols/approximate_equality_protocol.py Outdated
@@ -94,6 +95,11 @@ def approx_eq(val: Any, other: Any, *, atol: Union[int, float] = 1e-8) -> bool:
if isinstance(val, str):
return val == other

if isinstance(val, sympy.core.mul.Mul) or isinstance(
other, sympy.core.mul.Mul):
delta = sympy.simplify(sympy.Abs(other - val))
Copy link
Contributor

@balopat balopat Aug 6, 2020 •
edited

Choose a reason for hiding this comment

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

Got it - however, this is still not kosher. There might be a case where you can't evaluate to a number literal and then this question is undecidable. We should handle that and throw an error instead of silently returning False.

sympy.Abs(val - other).simplify().is_number should give you a good indication whether the simplification worked.

Please add a test case covering that too.

cirq/protocols/approximate_equality_protocol.py Outdated Show resolved Hide resolved
@tonybruguier-google
Copy link
Contributor Author

Fixes #2492

balopat
balopat suggested changes Aug 10, 2020
View reviewed changes
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

One last nit, thank you for the changes!

cirq/ops/eigen_gate_test.py Outdated Show resolved Hide resolved
cirq/protocols/approximate_equality_protocol.py Outdated Show resolved Hide resolved
@balopat balopat added the BREAKING CHANGE For pull requests that are important to mention in release notes. label Aug 10, 2020
@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 10, 2020
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 10, 2020
@CirqBot CirqBot merged commit 4bab2d2 into quantumlib:master Aug 10, 2020
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Aug 10, 2020
tonybruguier pushed a commit to tonybruguier/Cirq that referenced this pull request Aug 23, 2020
@tonybruguier-google
Approximately equal circuits with symbols not detected by cirq.approx…
30668cd 
…_eq (quantumlib#3195)

Approximately equal circuits with symbols not detected by cirq.approx_eq (Fixes quantumlib#3192.)

We should use sympy to make comparisons when available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@balopat balopat balopat approved these changes

Assignees
No one assigned
Labels
BREAKING CHANGE For pull requests that are important to mention in release notes. cla: yes Makes googlebot stop complaining.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Approximately equal circuits with symbols not detected by cirq.approx_eq
5 participants
@tonybruguier-google @balopat @googlebot @CirqBot @tonybruguier