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

Remove old graph structure during EquivalenceLibrary.set_entry #11959

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

jakelishman
Copy link
Member

Summary

The previous implementation of EquivalenceLibrary.set_entry changed the equivalence rules attached to particular nodes within the graph structure, but didn't correctly update the edges, so the graph was no longer representing the correct data, and queries within BasisTranslator would still use the old equivalence sets.

This correctly clears out the old rules and adds the new structure when set_entry is used. The inner private variable _rule_count is replaced with _rule_id, since it would actually be misleading for its use to decrement it; this could cause clashes, and what's actually intended and used is that it's an id for rules.

Details and comments

Fix #11958

@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 Mar 6, 2024
@jakelishman jakelishman added this to the 1.0.2 milestone Mar 6, 2024
@jakelishman jakelishman requested a review from a team as a code owner March 6, 2024 15:43
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Mar 6, 2024

Pull Request Test Coverage Report for Build 8178988151

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

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.006%) to 89.32%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 6 90.93%
qiskit/circuit/commutation_checker.py 6 95.95%
Totals Coverage Status
Change from base Build 8172358634: 0.006%
Covered Lines: 59145
Relevant Lines: 66217

💛 - Coveralls

The previous implementation of `EquivalenceLibrary.set_entry` changed
the equivalence rules attached to particular nodes within the graph
structure, but didn't correctly update the edges, so the graph was no
longer representing the correct data, and queries within
`BasisTranslator` would still use the old equivalence sets.

This correctly clears out the old rules and adds the new structure when
`set_entry` is used.  The inner private variable `_rule_count` is
replaced with `_rule_id`, since it would actually be misleading for its
use to decrement it; this could cause clashes, and what's actually
intended and used is that it's an id for rules.
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, but one question inline about the test coverage.

test/python/circuit/test_equivalence.py Show resolved Hide resolved
@eendebakpt
Copy link
Contributor

Tested locally and resolves the issue. Nice!

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 adding the extra test case

@mtreinish mtreinish added this pull request to the merge queue Mar 6, 2024
Merged via the queue into Qiskit:main with commit 67e0243 Mar 6, 2024
12 checks passed
mergify bot pushed a commit that referenced this pull request Mar 6, 2024
)

* Remove old graph structure during `EquivalenceLibrary.set_entry`

The previous implementation of `EquivalenceLibrary.set_entry` changed
the equivalence rules attached to particular nodes within the graph
structure, but didn't correctly update the edges, so the graph was no
longer representing the correct data, and queries within
`BasisTranslator` would still use the old equivalence sets.

This correctly clears out the old rules and adds the new structure when
`set_entry` is used.  The inner private variable `_rule_count` is
replaced with `_rule_id`, since it would actually be misleading for its
use to decrement it; this could cause clashes, and what's actually
intended and used is that it's an id for rules.

* Add test with parallel edges

(cherry picked from commit 67e0243)
@jakelishman jakelishman deleted the fix-equivalence-set-entry branch March 7, 2024 00:00
@jakelishman
Copy link
Member Author

@Mergifyio backport stable/0.46

Copy link
Contributor

mergify bot commented Mar 7, 2024

backport stable/0.46

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Mar 7, 2024
)

* Remove old graph structure during `EquivalenceLibrary.set_entry`

The previous implementation of `EquivalenceLibrary.set_entry` changed
the equivalence rules attached to particular nodes within the graph
structure, but didn't correctly update the edges, so the graph was no
longer representing the correct data, and queries within
`BasisTranslator` would still use the old equivalence sets.

This correctly clears out the old rules and adds the new structure when
`set_entry` is used.  The inner private variable `_rule_count` is
replaced with `_rule_id`, since it would actually be misleading for its
use to decrement it; this could cause clashes, and what's actually
intended and used is that it's an id for rules.

* Add test with parallel edges

(cherry picked from commit 67e0243)
github-merge-queue bot pushed a commit that referenced this pull request Mar 7, 2024
) (#11963)

* Remove old graph structure during `EquivalenceLibrary.set_entry`

The previous implementation of `EquivalenceLibrary.set_entry` changed
the equivalence rules attached to particular nodes within the graph
structure, but didn't correctly update the edges, so the graph was no
longer representing the correct data, and queries within
`BasisTranslator` would still use the old equivalence sets.

This correctly clears out the old rules and adds the new structure when
`set_entry` is used.  The inner private variable `_rule_count` is
replaced with `_rule_id`, since it would actually be misleading for its
use to decrement it; this could cause clashes, and what's actually
intended and used is that it's an id for rules.

* Add test with parallel edges

(cherry picked from commit 67e0243)

Co-authored-by: Jake Lishman <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Mar 7, 2024
) (#11962)

* Remove old graph structure during `EquivalenceLibrary.set_entry`

The previous implementation of `EquivalenceLibrary.set_entry` changed
the equivalence rules attached to particular nodes within the graph
structure, but didn't correctly update the edges, so the graph was no
longer representing the correct data, and queries within
`BasisTranslator` would still use the old equivalence sets.

This correctly clears out the old rules and adds the new structure when
`set_entry` is used.  The inner private variable `_rule_count` is
replaced with `_rule_id`, since it would actually be misleading for its
use to decrement it; this could cause clashes, and what's actually
intended and used is that it's an id for rules.

* Add test with parallel edges

(cherry picked from commit 67e0243)

Co-authored-by: Jake Lishman <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Mar 7, 2024
) (#11963)

* Remove old graph structure during `EquivalenceLibrary.set_entry`

The previous implementation of `EquivalenceLibrary.set_entry` changed
the equivalence rules attached to particular nodes within the graph
structure, but didn't correctly update the edges, so the graph was no
longer representing the correct data, and queries within
`BasisTranslator` would still use the old equivalence sets.

This correctly clears out the old rules and adds the new structure when
`set_entry` is used.  The inner private variable `_rule_count` is
replaced with `_rule_id`, since it would actually be misleading for its
use to decrement it; this could cause clashes, and what's actually
intended and used is that it's an id for rules.

* Add test with parallel edges

(cherry picked from commit 67e0243)

Co-authored-by: Jake Lishman <[email protected]>
IsmaelCesar pushed a commit to comp-quantica/qiskit-terra that referenced this pull request Mar 13, 2024
…kit#11959)

* Remove old graph structure during `EquivalenceLibrary.set_entry`

The previous implementation of `EquivalenceLibrary.set_entry` changed
the equivalence rules attached to particular nodes within the graph
structure, but didn't correctly update the edges, so the graph was no
longer representing the correct data, and queries within
`BasisTranslator` would still use the old equivalence sets.

This correctly clears out the old rules and adds the new structure when
`set_entry` is used.  The inner private variable `_rule_count` is
replaced with `_rule_id`, since it would actually be misleading for its
use to decrement it; this could cause clashes, and what's actually
intended and used is that it's an id for rules.

* Add test with parallel edges
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.

EquivalenceLibrary.set_entry does not clear old entries in graph
5 participants