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

Propagate DAGCircuit.name in ApplyLayout #13910

Merged
merged 4 commits into from
Feb 24, 2025

Conversation

jakelishman
Copy link
Member

This has always been absent, but transpile masked the problem by assigning the output circuit names by overwriting the output_names input kwarg if it wasn't set. generate_preset_pass_manager didn't have the same logic, so it was observable that ApplyLayout didn't propagate the name.

We could have added similar name-propagation logic to generate_preset_pass_manager, but really the bug is in ApplyLayout; passes should propagate the metadata.

Summary

Details and comments

This has always been absent, but `transpile` masked the problem by
assigning the output circuit names by overwriting the `output_names`
input kwarg if it wasn't set.  `generate_preset_pass_manager` didn't
have the same logic, so it was observable that `ApplyLayout` didn't
propagate the name.

We could have added similar name-propagation logic to
`generate_preset_pass_manager`, but really the bug is in `ApplyLayout`;
passes _should_ propagate the metadata.
@jakelishman jakelishman added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Feb 21, 2025
@jakelishman jakelishman added this to the 1.4.1 milestone Feb 21, 2025
@jakelishman jakelishman requested a review from a team as a code owner February 21, 2025 22:33
@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 Feb 21, 2025

Pull Request Test Coverage Report for Build 13507255486

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 36 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.04%) to 87.984%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 6 91.98%
crates/qasm2/src/parse.rs 30 95.76%
Totals Coverage Status
Change from base Build 13506447276: -0.04%
Covered Lines: 77941
Relevant Lines: 88585

💛 - Coveralls

Copy link
Contributor

@jlapeyre jlapeyre left a comment

Choose a reason for hiding this comment

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

I made one minor correction and a question on a minor point. Other than that it looks good.

jakelishman and others added 2 commits February 24, 2025 12:15
@kevinhartman kevinhartman added this pull request to the merge queue Feb 24, 2025
@mtreinish mtreinish removed this pull request from the merge queue due to a manual request Feb 24, 2025
@mtreinish mtreinish dismissed jlapeyre’s stale review February 24, 2025 21:18

Comments addressed

@jakelishman jakelishman added this pull request to the merge queue Feb 24, 2025
Merged via the queue into Qiskit:main with commit 29aa2bd Feb 24, 2025
19 checks passed
mergify bot pushed a commit that referenced this pull request Feb 24, 2025
* Propagate `DAGCircuit.name` in `ApplyLayout`

This has always been absent, but `transpile` masked the problem by
assigning the output circuit names by overwriting the `output_names`
input kwarg if it wasn't set.  `generate_preset_pass_manager` didn't
have the same logic, so it was observable that `ApplyLayout` didn't
propagate the name.

We could have added similar name-propagation logic to
`generate_preset_pass_manager`, but really the bug is in `ApplyLayout`;
passes _should_ propagate the metadata.

* Fix typo

Co-authored-by: John Lapeyre <[email protected]>

* Remove redunant set

---------

Co-authored-by: John Lapeyre <[email protected]>
(cherry picked from commit 29aa2bd)

# Conflicts:
#	test/python/transpiler/test_preset_passmanagers.py
@jakelishman jakelishman deleted the fix-name-transpile branch February 24, 2025 23:32
jakelishman added a commit that referenced this pull request Feb 25, 2025
* Propagate `DAGCircuit.name` in `ApplyLayout`

This has always been absent, but `transpile` masked the problem by
assigning the output circuit names by overwriting the `output_names`
input kwarg if it wasn't set.  `generate_preset_pass_manager` didn't
have the same logic, so it was observable that `ApplyLayout` didn't
propagate the name.

We could have added similar name-propagation logic to
`generate_preset_pass_manager`, but really the bug is in `ApplyLayout`;
passes _should_ propagate the metadata.

* Fix typo

Co-authored-by: John Lapeyre <[email protected]>

* Remove redunant set

---------

Co-authored-by: John Lapeyre <[email protected]>
(cherry picked from commit 29aa2bd)
jakelishman added a commit that referenced this pull request Feb 25, 2025
* Propagate `DAGCircuit.name` in `ApplyLayout`

This has always been absent, but `transpile` masked the problem by
assigning the output circuit names by overwriting the `output_names`
input kwarg if it wasn't set.  `generate_preset_pass_manager` didn't
have the same logic, so it was observable that `ApplyLayout` didn't
propagate the name.

We could have added similar name-propagation logic to
`generate_preset_pass_manager`, but really the bug is in `ApplyLayout`;
passes _should_ propagate the metadata.

* Fix typo

Co-authored-by: John Lapeyre <[email protected]>

* Remove redunant set

---------

Co-authored-by: John Lapeyre <[email protected]>
(cherry picked from commit 29aa2bd)
github-merge-queue bot pushed a commit that referenced this pull request Feb 25, 2025
* Propagate `DAGCircuit.name` in `ApplyLayout`

This has always been absent, but `transpile` masked the problem by
assigning the output circuit names by overwriting the `output_names`
input kwarg if it wasn't set.  `generate_preset_pass_manager` didn't
have the same logic, so it was observable that `ApplyLayout` didn't
propagate the name.

We could have added similar name-propagation logic to
`generate_preset_pass_manager`, but really the bug is in `ApplyLayout`;
passes _should_ propagate the metadata.

* Fix typo

Co-authored-by: John Lapeyre <[email protected]>

* Remove redunant set

---------

Co-authored-by: John Lapeyre <[email protected]>
(cherry picked from commit 29aa2bd)

Co-authored-by: Jake Lishman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: transpiler Issues and PRs related to Transpiler stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants