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

Add assert_decompose_ends_at_default_gateset consistency test #5079

Conversation

tanujkhattar
Copy link
Collaborator

  • Part-1 of Make sure cirq gates and operations decompose to XPow/YPow/ZPow/CZPow + Measurement #5057
  • The PR adds cirq.testing.assert_decompose_ends_at_default_gateset which checks that if the given object (gate / operation) has a decompose method defined, then it should always end up decomposing to the cirq default gateset.
  • In subsequent PRs, this method will also be added to cirq.testing.assert_implements_consistent_protocols to extend the coverage of this test across all cirq objects.

cc @MichaelBroughton

@tanujkhattar tanujkhattar requested a review from a team as a code owner March 15, 2022 21:20
@CirqBot CirqBot added the size: M 50< lines changed <250 label Mar 15, 2022
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

Code looks good, a few high level questions.

"""Ensures that all cirq gate decompositions end at the default cirq gateset."""

# pylint: disable=unused-variable
__tracebackhide__ = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this for ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is useful to hide the traceback of assertion calls, in order to avoid clutter in user code if tests fail.

It's used as a general pattern across all cirq/testing/ methods. I've removed it for now as I think the assertion trace is very useful here, especially while this is still a WIP and further decompositions are being added in subsequent PRs.

Comment on lines 58 to 59
if protocols.is_parameterized(val):
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't decompose work for gates that have symbols in them too ? Maybe it would be best here to assert that this is being called on a parameter free val so one doesn't accidentally mistake accidentally putting in a symbol for success. WDYT ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have general purpose analytical decompositions for parameterized gates which makes it much harder to ensure that the decompositions of parameterize gates ends up in the default gateset (eg: parameterized controlled gates and matrix gates).

However, I gave some more thought to it and instead of ignoring all parameterized gates or all gates without a decompose implemented (done below), I've added a dedicated method _known_gate_with_no_decomposition, which should be used as an escape patch to ignore gates which are known to not have automatic decompositions into the target gateset.

All cases (eg: parameterized controlled gates) for which we do not support automatic decomposition, will be added to this method in subsequent PRs.

Comment on lines 62 to 64
if dec_once is None:
# _decompose_ is NotImplemented, so silently return.
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as above, if not implemented wouldn't we want to fail loudly ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed the feedback, see my comment above.

Copy link
Collaborator Author

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

I've restructured the code and added a dedicated method _known_gate_with_no_decomposition, where we will add all known cases where default decompositions don't exist, in future PRs.

This is ready for another look.

Comment on lines 58 to 59
if protocols.is_parameterized(val):
return
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have general purpose analytical decompositions for parameterized gates which makes it much harder to ensure that the decompositions of parameterize gates ends up in the default gateset (eg: parameterized controlled gates and matrix gates).

However, I gave some more thought to it and instead of ignoring all parameterized gates or all gates without a decompose implemented (done below), I've added a dedicated method _known_gate_with_no_decomposition, which should be used as an escape patch to ignore gates which are known to not have automatic decompositions into the target gateset.

All cases (eg: parameterized controlled gates) for which we do not support automatic decomposition, will be added to this method in subsequent PRs.

Comment on lines 62 to 64
if dec_once is None:
# _decompose_ is NotImplemented, so silently return.
return
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed the feedback, see my comment above.

"""Ensures that all cirq gate decompositions end at the default cirq gateset."""

# pylint: disable=unused-variable
__tracebackhide__ = True
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is useful to hide the traceback of assertion calls, in order to avoid clutter in user code if tests fail.

It's used as a general pattern across all cirq/testing/ methods. I've removed it for now as I think the assertion trace is very useful here, especially while this is still a WIP and further decompositions are being added in subsequent PRs.

@tanujkhattar tanujkhattar added the priority/high This is something that should get done soon, e.g. within a month. label Mar 16, 2022
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Mar 16, 2022
@tanujkhattar tanujkhattar merged commit 765ccfe into quantumlib:master Mar 16, 2022
@tanujkhattar tanujkhattar deleted the assert_decompose_ends_at_default_gateset branch March 16, 2022 21:05
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…mlib#5079)

* Add assert_decompose_ends_at_default_gateset consistency test

* Refactor assert_decompose_ends_at_default_gateset to provide hook to ignore gates without known decompositions
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
…mlib#5079)

* Add assert_decompose_ends_at_default_gateset consistency test

* Refactor assert_decompose_ends_at_default_gateset to provide hook to ignore gates without known decompositions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Tells CirqBot to sync and merge this PR. (If it's running.) priority/high This is something that should get done soon, e.g. within a month. size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants