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

Deprecate loose custom basis gates in preset pm pipieline #13394

Merged
merged 7 commits into from
Nov 6, 2024

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Nov 5, 2024

Summary

This PR is a spinoff of #12850 that introduces the deprecation warning for loose custom basis gates as an input to transpile/generate_preset_pm. This is a pre-requisite for eventually merging #12850. Moving forward, users can still provide custom basis gates but they need to be included as part of a Target instance.

Details and comments

The PR also modifies a few oversights found in generate_preset_pass_manager that didn't have user impact but should be corrected to set the stage for a target-only pipeline:

  • we were "over-skipping" the target by having a condition with an or instead of an and (fixed in L453/466)
  • control flow ops were not included by default in the custom name mapping (this would raise a warning if control flow ops were provided as loose basis gates). This didn't cause issues because the target was often skipped with the previous oversight.

@ElePT ElePT added the Changelog: Deprecation Include in "Deprecated" section of changelog label Nov 5, 2024
@ElePT ElePT added this to the 1.3.0 milestone Nov 5, 2024
@ElePT ElePT requested a review from a team as a code owner November 5, 2024 10:22
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Nov 5, 2024

Pull Request Test Coverage Report for Build 11702502035

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

  • 42 of 42 (100.0%) changed or added relevant lines in 5 files are covered.
  • 147 unchanged lines in 15 files lost coverage.
  • Overall coverage increased (+0.06%) to 88.829%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 92.2%
crates/qasm2/src/expr.rs 1 94.02%
qiskit/circuit/library/generalized_gates/gms.py 2 94.44%
qiskit/circuit/library/generalized_gates/rv.py 2 84.62%
qiskit/transpiler/preset_passmanagers/builtin_plugins.py 2 95.74%
qiskit/primitives/backend_sampler_v2.py 2 98.53%
qiskit/circuit/library/generalized_gates/diagonal.py 3 95.16%
qiskit/circuit/library/generalized_gates/permutation.py 3 92.73%
crates/qasm2/src/lex.rs 8 91.73%
qiskit/circuit/library/grover_operator.py 9 92.86%
Totals Coverage Status
Change from base Build 11669445061: 0.06%
Covered Lines: 77100
Relevant Lines: 86796

💛 - Coveralls

Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks and good catch with the target-skipping condition! I have couple small comments, one is important.

@@ -433,9 +448,14 @@ def generate_preset_pass_manager(
def _parse_basis_gates(basis_gates, backend, inst_map, skip_target):
name_mapping = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code now

@ElePT
Copy link
Contributor Author

ElePT commented Nov 5, 2024

Thanks for the review! Really good catch with transpile. This actually made me notice that it was used internally in the Quantum Shannon Decomposer, so I had to do a small refactoring there. The code should look good now!

@ElePT ElePT requested review from eggerdj and wshanks as code owners November 6, 2024 10:09
Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks for all the changes and for handling the newly exposed deprecation warnings. I've left one question for you to confirm in qsd.py; assuming you will, I think this PR is good to go hence already approving.

circ, basis_gates=["u", "cx", "qsd2q"], optimization_level=0, qubits_initially_zero=False
)
hls = HighLevelSynthesis(basis_gates=["u", "cx", "qsd2q"], qubits_initially_zero=False)
ccirc = hls(circ)
Copy link
Contributor

@eliarbel eliarbel Nov 6, 2024

Choose a reason for hiding this comment

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

Previously transpile built a flow containing UnitarySynthesis, HighLevelSynthesis and BasisTranslator. Looking at qs_decomposition seems that this change is safe in the sense that HighLevelSynthesis should be sufficient. Can you just confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe it is, I also briefly confirmed it with @Cryoris.

@ElePT ElePT added this pull request to the merge queue Nov 6, 2024
Merged via the queue into Qiskit:main with commit 3be806d Nov 6, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants