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 grover_operator function #13365

Merged
merged 5 commits into from
Nov 5, 2024
Merged

Add grover_operator function #13365

merged 5 commits into from
Nov 5, 2024

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Oct 24, 2024

Summary

Part of #13046: The grover_operator as function.

Details and comments

This function is similar to the GroverOperator, but does not require choosing the implementation of the multi-controlled X gate a-priori and let's the compiler choose the optimal decomposition instead.

@Cryoris Cryoris added this to the 1.3.0 milestone Oct 24, 2024
@Cryoris Cryoris requested a review from a team as a code owner October 24, 2024 08:52
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

@coveralls
Copy link

coveralls commented Oct 24, 2024

Pull Request Test Coverage Report for Build 11688478692

Details

  • 40 of 43 (93.02%) changed or added relevant lines in 2 files are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.005%) to 88.768%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/library/grover_operator.py 39 42 92.86%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 3 92.48%
crates/qasm2/src/parse.rs 6 96.69%
Totals Coverage Status
Change from base Build 11683622570: 0.005%
Covered Lines: 76813
Relevant Lines: 86532

💛 - Coveralls

@ElePT ElePT self-assigned this Oct 29, 2024
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

PR code looks good, I just left a few documentation-related comments (plus reminder to fix the merge conflict).

Comment on lines 162 to 163
For a large number of qubits, the multi-controlled X gate used for the zero-reflection
can be synthesized in different fashion. Depending on the number of available qubits,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For a large number of qubits, the multi-controlled X gate used for the zero-reflection
can be synthesized in different fashion. Depending on the number of available qubits,
For a large number of qubits, the multi-controlled X gate used for the zero-reflection
can be synthesized in different fashions. Depending on the number of available qubits,

Comment on lines 182 to 184
# add extra bits that can be used as scratch space
grover_op.add_bits([Qubit() for _ in range(num_qubits)])
print("2q depth w/ scratch qubits:", tqc.depth(filter_function=is_2q)) # < 100
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you retranspile after adding the extra bits in this example?

Comment on lines +410 to +411
The :func:`.grover_operator` implements the same functionality but keeping the
:class:`.MCXGate` abstract, such that the compiler may choose the optimal decomposition.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about:

Suggested change
The :func:`.grover_operator` implements the same functionality but keeping the
:class:`.MCXGate` abstract, such that the compiler may choose the optimal decomposition.
The :func:`.grover_operator` implements the same functionality but keeping the
:class:`.MCXGate` abstract, such that the compiler may choose the optimal decomposition.
We recommend using :func:`.grover_operator` for performance reasons.

Grover's algorithm and amplitude estimation/amplification. This function is similar to
:class:`.GroverOperator`, but does not require choosing the implementation of the
multi-controlled X gate a-priori and let's the compiler choose the optimal decomposition
instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the amount of examples you have in the docstring, I would copy-paste one into the reno for quick reference.

@@ -125,9 +144,10 @@ def test_custom_zero_reflection(self):
expected.h(0) # state_in is H
expected.compose(zero_reflection, inplace=True)
expected.h(0)
self.assertEqual(expected, grover_op.decompose())
self.assertEqual(expected, grover_op if use_function else grover_op.decompose())
Copy link
Contributor

Choose a reason for hiding this comment

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

This tests makes me think that we should mention in the reno and docstring that the output of the new constructor is a series of gates instead of a "black box". This is actually the main user-facing difference I see (besides the mcx detail)

@ShellyGarion ShellyGarion added the Changelog: New Feature Include in the "Added" section of the changelog label Nov 5, 2024
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for applying the REviwe comments :)

@ElePT ElePT enabled auto-merge November 5, 2024 16:32
@ElePT ElePT added this pull request to the merge queue Nov 5, 2024
Merged via the queue into Qiskit:main with commit 185383d Nov 5, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants