Skip to content

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Jul 22, 2024

Summary

This PR fixes a couple of issues that should have been addressed in #12185. Found while working on #12762 and changes built on top of it (must be rebased).

Details and comments

Copy link
Contributor

mergify bot commented Jul 22, 2024

⚠️ The sha of the head commit of this PR conflicts with #12762. Mergify cannot evaluate rules on this PR. ⚠️

@coveralls
Copy link

coveralls commented Jul 22, 2024

Pull Request Test Coverage Report for Build 10075733597

Details

  • 18 of 19 (94.74%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 89.924%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/preset_passmanagers/generate_preset_pass_manager.py 18 19 94.74%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 3 92.37%
Totals Coverage Status
Change from base Build 10075234532: 0.03%
Covered Lines: 65864
Relevant Lines: 73244

💛 - Coveralls

@ElePT ElePT added this to the 1.2.0 milestone Jul 23, 2024
@ElePT ElePT force-pushed the gppm-oversights branch from 05c1f12 to 43cd9a4 Compare July 24, 2024 11:18
…e not migrated from transpile to generate_preset_pm with the others.
@ElePT ElePT force-pushed the gppm-oversights branch from 43cd9a4 to e8eb508 Compare July 24, 2024 11:20
@ElePT ElePT marked this pull request as ready for review July 24, 2024 11:24
@ElePT ElePT requested a review from a team as a code owner July 24, 2024 11:24
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

Comment on lines -342 to -343
initial_layout = _parse_initial_layout(initial_layout)
approximation_degree = _parse_approximation_degree(approximation_degree)
Copy link
Contributor Author

@ElePT ElePT Jul 24, 2024

Choose a reason for hiding this comment

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

Before this PR, setting a custom initial_layout or approximation_degree could lead to different results on generate_preset_pass_manager vs transpile, while the rest of inputs would be consistent.

Comment on lines +17 to +20
- |
A series of input-handling inconsistencies between :func:`.transpile` and :func:`.generate_preset_pass_manager`
have been fixed. These inconsistencies would lead to different transpilation outputs for the same inputs,
or :func:`.generate_preset_pass_manager` failing for certain input combinations accepted by :func:`.transpile`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the inconsistencies were fixed in the previous PR but we didn't have any reno to refer to. Given that there have been issues about these inconsistencies, I think it would be good to mention that they will no longer happen (but we can adjust the wording).

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, this is straightforward enough and inline with the other changes made.

@mtreinish mtreinish added this pull request to the merge queue Jul 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2024
@mtreinish mtreinish added this pull request to the merge queue Jul 24, 2024
Merged via the queue into Qiskit:main with commit d8903f3 Jul 24, 2024
15 checks passed
@ElePT ElePT added the Changelog: None Do not include in changelog label Jul 30, 2024
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
* Fix oversight from Qiskit#12185 where two input parsing functions were not migrated from transpile to generate_preset_pm  with the others.

* Add fix to reno from Qiskit#12185
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants