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

Bugfix in _strat_has_stabilizer_effect_from_decompose #6467

Merged

Conversation

tanujkhattar
Copy link
Collaborator

Fixes quantumlib/Qualtran#665

@NoureldinYosri Do you remember why you added the following check?

    qid_shape = qid_shape_protocol.qid_shape(val, default=None)
    if qid_shape is None or len(qid_shape) <= 3:
        return None

@tanujkhattar tanujkhattar requested review from vtomole, cduck and a team as code owners February 15, 2024 23:13
@CirqBot CirqBot added the size: S 10< lines changed <50 label Feb 15, 2024
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6a40da5) 97.82% compared to head (fb3c062) 97.82%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6467   +/-   ##
=======================================
  Coverage   97.82%   97.82%           
=======================================
  Files        1115     1115           
  Lines       97448    97452    +4     
=======================================
+ Hits        95327    95331    +4     
  Misses       2121     2121           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NoureldinYosri
Copy link
Collaborator

NoureldinYosri commented Feb 16, 2024

to stop the recursion. if it has 3 or less qubits then the from_unitary strategy should have caught it

if (
qid_shape is None
or len(qid_shape) > 3
or qid_shape != (2,) * len(qid_shape)
or not has_unitary_protocol.has_unitary(val)
):
return None
unitary = unitary_protocol.unitary(val)

if it outputs the wrong result then the bug will be there not here.

is there an example of this producing the wrong result?

@tanujkhattar
Copy link
Collaborator Author

is there an example of this producing the wrong result?

The bug was in decompose_protocol.decompose_once(val). That works only when val knows the qubits it acts upon (so operations, moments, circuits) but fails for gates. _try_decompose_into_operations_and_qubits works in the general case. I've added a tests that was failing earlier but passes now

to stop the recursion

I think the check is redundant, because the control flow would reach the decomposition strategy only when it couldn't figure out the answer using the first 3 strategies and that would imply a scenario where, for example, the operation decomposes into non unitary operations (eg: measurements + cliffords) and therefore we should continue to check decomposition. An example in the test shows a scenario where _unitary_ is not defined but decomposition and _has_stabilizer_effect_ is.

@tanujkhattar
Copy link
Collaborator Author

Let's wait and not merge right now, I need to run some more tests

@tanujkhattar tanujkhattar merged commit d432730 into quantumlib:main Feb 16, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cirq-style t_complexity protocol doesn't work with latest version of cirq
3 participants