Skip to content

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Jul 11, 2024

Summary

generate_preset_pass_manager deserves a proper home. This PR moves this function from the __init__ file in transpiler/preset_passmanagers to a standalone file in the same directory, generate_preset_pass_manager.py.
Note that this move doesn't affect the public interface or documented import path, but it makes it easier to find the source code when traversing the repo.

Details and comments

The PR now adds a global import path for generate_preset_pass_manager and contributes to #11897.

@ElePT ElePT requested a review from a team as a code owner July 11, 2024 15:13
@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 Jul 11, 2024

Pull Request Test Coverage Report for Build 10073131207

Details

  • 176 of 189 (93.12%) changed or added relevant lines in 25 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 89.948%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/preset_passmanagers/generate_preset_pass_manager.py 120 133 90.23%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 93.64%
Totals Coverage Status
Change from base Build 10067705422: 0.02%
Covered Lines: 65861
Relevant Lines: 73221

💛 - Coveralls

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.

The other thing we should do as part of this is expose this from qiskit/__init__.py since it's "commonly" used.

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, thanks for the quick update and adding the top level import too. Just a few small nits in the release note, but otherwise this is ready to merge.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@ElePT ElePT added this to the 1.2.0 milestone Jul 15, 2024
mtreinish
mtreinish previously approved these changes Jul 15, 2024
@mtreinish mtreinish added this pull request to the merge queue Jul 15, 2024
@jakelishman
Copy link
Member

jakelishman commented Jul 15, 2024

Might we be amenable to making this name available from qiskit.transpiler as well?

@ElePT ElePT removed this pull request from the merge queue due to a manual request Jul 15, 2024
@ElePT
Copy link
Contributor Author

ElePT commented Jul 16, 2024

Might we be amenable to making this name available from qiskit.transpiler as well?

@jakelishman Makes sense to me, added path in ebb48ac.

jakelishman
jakelishman previously approved these changes Jul 16, 2024
Copy link
Member

@jakelishman jakelishman 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!

@jakelishman jakelishman enabled auto-merge July 16, 2024 10:03
jakelishman
jakelishman previously approved these changes Jul 16, 2024
@ElePT ElePT force-pushed the move-generate-preset-pm-to-new-home branch from bc48357 to 6eb5dc0 Compare July 22, 2024 09:31
@ElePT ElePT force-pushed the move-generate-preset-pm-to-new-home branch from 498842b to 9069dde Compare July 22, 2024 09:59
@ElePT
Copy link
Contributor Author

ElePT commented Jul 22, 2024

While dealing with the cyclic imports I realized that, in #12185, I hadn't migrated two input handling functions (the ones for layout and approximation_degree) to generate_preset_pass_manager to keep a consistent behavior between transpile and generate ppm. Fixed in 2bac8f1.

Now that I think of it, maybe I should open an independent PR with these fixes, it might be a bit clearer.

Edit: changes extracted to #12799.

@ElePT ElePT added Changelog: New Feature Include in the "Added" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Jul 22, 2024
@ElePT ElePT force-pushed the move-generate-preset-pm-to-new-home branch from e039638 to 6eb5dc0 Compare July 22, 2024 14:46
@mtreinish mtreinish self-assigned this Jul 23, 2024
mtreinish
mtreinish previously approved these changes Jul 23, 2024
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, thanks for getting to the bottom of those hairy import cycles

…185f3f738b.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@jakelishman jakelishman added this pull request to the merge queue Jul 24, 2024
Merged via the queue into Qiskit:main with commit e09d345 Jul 24, 2024
15 checks passed
ElePT added a commit to mtreinish/qiskit-core that referenced this pull request Jul 24, 2024
* Move generate_preset_pass_manager to generate_preset_pass_manager.py

* Add top-level import and reno

* Apply suggestions from Matt's code review

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Add import path from qiskit.transpiler too

* Fix cyclic import issue

* Fix cyclic import issue in tools/pgo_scripts

* Address cyclic import issues. Reorder imports alphabetically when relevant.

* Fix docstring issue

* Update releasenotes/notes/add-generate-preset-pm-global-import-efb12f185f3f738b.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
* Move generate_preset_pass_manager to generate_preset_pass_manager.py

* Add top-level import and reno

* Apply suggestions from Matt's code review

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Add import path from qiskit.transpiler too

* Fix cyclic import issue

* Fix cyclic import issue in tools/pgo_scripts

* Address cyclic import issues. Reorder imports alphabetically when relevant.

* Fix docstring issue

* Update releasenotes/notes/add-generate-preset-pm-global-import-efb12f185f3f738b.yaml

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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 mod: transpiler Issues and PRs related to Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants