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

Move mcx synthesis methods with ancillas to the synthesis library #12904

Merged
merged 20 commits into from
Aug 11, 2024

Conversation

ShellyGarion
Copy link
Member

@ShellyGarion ShellyGarion commented Aug 5, 2024

Summary

See details in issue #12863.
In this PR we transfer the mcx synthesis methods with clean and dirty ancillas to the synthesis library.

Details and comments

Added 3 functions in the qiskit/synthesis/multi-controlled library:

  • synth_mcx_1_clean_b95
  • synth_n_dirty_i15
  • synth_n_clean_m15

@ShellyGarion ShellyGarion added this to the 1.3.0 milestone Aug 5, 2024
@ShellyGarion ShellyGarion requested a review from Cryoris August 5, 2024 09:37
@ShellyGarion ShellyGarion requested review from alexanderivrii and a team as code owners August 5, 2024 09:37
@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 Aug 5, 2024

Pull Request Test Coverage Report for Build 10338784669

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 112 of 116 (96.55%) changed or added relevant lines in 4 files are covered.
  • 299 unchanged lines in 15 files lost coverage.
  • Overall coverage decreased (-0.006%) to 89.745%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/synthesis/multi_controlled/mcx_with_ancillas_synth.py 101 105 96.19%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
qiskit/primitives/statevector_estimator.py 3 94.44%
crates/qasm2/src/lex.rs 3 91.98%
qiskit/primitives/estimator.py 3 96.2%
qiskit/transpiler/passes/optimization/consolidate_blocks.py 5 95.28%
crates/accelerate/src/synthesis/clifford/greedy_synthesis.rs 5 97.34%
crates/qasm2/src/parse.rs 6 97.61%
crates/circuit/src/parameter_table.rs 7 93.27%
qiskit/transpiler/passes/basis/basis_translator.py 8 97.44%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py 12 96.45%
Totals Coverage Status
Change from base Build 10222290210: -0.006%
Covered Lines: 67411
Relevant Lines: 75114

💛 - Coveralls

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the effort! Should we already pending-deprecate the gate classes and refer to the synthesis functions? Once we have the MCX HLS by Sasha we can extend the deprecation message to state that users can also use the HLS plugin.

@ShellyGarion
Copy link
Member Author

Note that in commit 7b68e57 I had to revert the new synthesized circuit names since QASM based tests are failing.
Should I update the names and fix the tests? or is it considered breaking API?

Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! The new function names, such as synth_mcx_n_dirty_i15, synth_mcx_1_clean_b95, etc. are reasonable. One small question: once we integrate these into plugins, would it make sense for the plugin names to be n_dirty_i15, 1_clean_b95, etc. as in hls = HLSConfig(mcx=['1_clean_b95`])?

@ShellyGarion ShellyGarion changed the title [WIP] Move mcx synthesis methods with ancillas to the synthesis library Move mcx synthesis methods with ancillas to the synthesis library Aug 11, 2024
@ShellyGarion ShellyGarion added the Changelog: New Feature Include in the "Added" section of the changelog label Aug 11, 2024
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks @ShellyGarion! This looks very good. I had a few minor comments about explicitly adding the information about the total numbers of qubits to the new synthesis function docstrings and to the release notes , moving imports to the top-level, and providing more explicit links to the papers in the release notes. After this is addressed, I would be happy to merge.

Comment on lines 63 to 64
# pylint: disable=cyclic-import
from qiskit.circuit.library.standard_gates.x import C3XGate
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to import this at the top-level, and instead add disable=cyclic-import to x.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, this will not work, since there is a real cyclic import here

Copy link
Contributor

@alexanderivrii alexanderivrii 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 making the changes!

@alexanderivrii alexanderivrii added this pull request to the merge queue Aug 11, 2024
Merged via the queue into Qiskit:main with commit c7e7016 Aug 11, 2024
15 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 synthesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants