Skip to content

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
Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
@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 <jlapeyre@users.noreply.github.com>

* Remove redunant set

---------

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
(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 <jlapeyre@users.noreply.github.com>

* Remove redunant set

---------

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
(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 <jlapeyre@users.noreply.github.com>

* Remove redunant set

---------

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
(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 <jlapeyre@users.noreply.github.com>

* Remove redunant set

---------

Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com>
(cherry picked from commit 29aa2bd)

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
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