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

Discard cache for standard gates in assign_parameters (backport #13557) #13559

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Dec 12, 2024

Summary

This is an initial fix to #13504, and should precede #13543.

The following commits discard the cached gate when assigning parameters for a standard gate rather than trying to modify the Python object.

Details and comments

A more in-depth fix is present in #13543, but since it is such a big change, we've decided to include this smaller fix to allow more time to re-evaluate our approach.

When retrieving a PackedInstruction from the EquivalenceLibrary during the compose_transforms phase of the BasisTranslator the parameter modifications done in other places are not reflected on the cached gate, therefore leading to issues when running QISKIT_PARALLEL=TRUE due to a modified object reference when calling CircuitData::assign_parameters().

In the case of a standard gate, we were not removing the old cache but instead modifying it in place (which would not result in the expected outcome), while for other gate instances, we rebuild the non-cached item. Now we instead discard the cache in the StandardGate case too, getting rid of the stale shared reference.


This is an automatic backport of pull request #13557 done by [Mergify](https://mergify.com).

* Fix: Discard cache when assigning new parameters in `CircuitData`.

* Docs: Add release note

* Test: Add test case

* Docs: Add relevant comment

(cherry picked from commit 17a2ccc)
@mergify mergify bot requested a review from a team as a code owner December 12, 2024 10:01
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

  • @Qiskit/terra-core

@github-actions github-actions bot added priority: high Changelog: Bugfix Include in the "Fixed" section of the changelog Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Dec 12, 2024
@github-actions github-actions bot added this to the 1.3.1 milestone Dec 12, 2024
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

I double checked the original issue (#13504) with this fix on stable/1.3 as well

@Cryoris Cryoris enabled auto-merge December 12, 2024 10:20
@coveralls
Copy link

Pull Request Test Coverage Report for Build 12294038102

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 88.92%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 92.2%
crates/qasm2/src/lex.rs 3 92.73%
Totals Coverage Status
Change from base Build 12279362746: 0.02%
Covered Lines: 79091
Relevant Lines: 88946

💛 - Coveralls

@Cryoris Cryoris added this pull request to the merge queue Dec 12, 2024
Merged via the queue into stable/1.3 with commit 1e32313 Dec 12, 2024
20 checks passed
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: circuit Related to the core of the `QuantumCircuit` class or the circuit library priority: high Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants