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

Add representation of storage-owning Var nodes #10944

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented Oct 2, 2023

Summary

This adds the representation of expr.Var nodes that own their own storage locations, and consequently are not backed by existing Qiskit objects (Clbit or ClassicalRegister). This is the base of the ability for Qiskit to represent manual classical-value storage in QuantumCircuit, and the base for how manual storage will be implemented.

Details and comments

Close #10923.

The release note will be added as part of closing epic #10922.

@jakelishman jakelishman added the mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library label Oct 2, 2023
@jakelishman jakelishman added this to the 0.45.0 milestone Oct 2, 2023
@jakelishman jakelishman requested a review from a team as a code owner October 2, 2023 12:19
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

This adds the representation of `expr.Var` nodes that own their own
storage locations, and consequently are not backed by existing Qiskit
objects (`Clbit` or `ClassicalRegister`).  This is the base of the
ability for Qiskit to represent manual classical-value storage in
`QuantumCircuit`, and the base for how manual storage will be
implemented.
@coveralls
Copy link

coveralls commented Nov 25, 2023

Pull Request Test Coverage Report for Build 7019349075

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 9 of 12 (75.0%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.2%) to 87.254%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/classical/expr/expr.py 9 12 75.0%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 2 92.68%
qiskit/circuit/instruction.py 2 94.83%
crates/qasm2/src/parse.rs 6 97.6%
Totals Coverage Status
Change from base Build 6985085373: 0.2%
Covered Lines: 60788
Relevant Lines: 69668

💛 - 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.

Overall this LGTM just a few comments on the docs. Did you want to include a release note for this change to explain the new features on the Var class? Or are you doing a single combined note at the end (I feel like it might be easier to have one file that's incrementally updated for each PR).

Edit: NM about the release note, I see that was addressed in the PR summary.

qiskit/circuit/classical/expr/__init__.py Outdated Show resolved Hide resolved
qiskit/circuit/classical/expr/__init__.py Outdated Show resolved Hide resolved
qiskit/circuit/classical/expr/__init__.py Outdated Show resolved Hide resolved
qiskit/circuit/classical/expr/expr.py Show resolved Hide resolved
Co-authored-by: Matthew Treinish <[email protected]>
@mtreinish mtreinish added this pull request to the merge queue Nov 28, 2023
Merged via the queue into Qiskit:main with commit 50e8137 Nov 28, 2023
14 checks passed
@jakelishman jakelishman deleted the var/repr branch November 30, 2023 13:56
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request Jan 29, 2024
This commit is a roll-up reversion of the following commits (PR):

* This reverts commit a79e879 (Qiskit#10977)
* This reverts commit ba161e9 (Qiskit#10974)
* This reverts commit 50e8137 (Qiskit#10944)

This is being done to clear the 1.0 branch of memory-owning `expr.Var`
variables and `Store` instructions.  It should be re-applied once 1.0 is
released.

The individual reverts had conflicts, since various other large-scale
changes include wrap-ups of the changes to be reverted.  This reversion
should have handled all that, and other places where standalone `Var`
nodes were referenced (such as in "See also" sections), even if not
used.
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request Jan 29, 2024
This commit is a roll-up reversion of the following commits (PR):

* commit a79e879 (Qiskit#10977)
* commit ba161e9 (Qiskit#10974)
* commit 50e8137 (Qiskit#10944)

This is being done to clear the 1.0 branch of memory-owning `expr.Var`
variables and `Store` instructions.  It should be re-applied once 1.0 is
released.

This wasn't done by individual `revert` operations, because there were
also significant structural changes introduced in those PRs that were
very valid and should be maintained.  Cross-references to `Var` nodes
from other functions have been removed for now.

Making `Var` and `types.Type` hashable is maintained, as is the
`Var.standalone` function, in order to prepare the ground for the
inclusion of proper `Var` nodes in a minor release.
github-merge-queue bot pushed a commit that referenced this pull request Jan 29, 2024
This commit is a roll-up reversion of the following commits (PR):

* commit a79e879 (#10977)
* commit ba161e9 (#10974)
* commit 50e8137 (#10944)

This is being done to clear the 1.0 branch of memory-owning `expr.Var`
variables and `Store` instructions.  It should be re-applied once 1.0 is
released.

This wasn't done by individual `revert` operations, because there were
also significant structural changes introduced in those PRs that were
very valid and should be maintained.  Cross-references to `Var` nodes
from other functions have been removed for now.

Making `Var` and `types.Type` hashable is maintained, as is the
`Var.standalone` function, in order to prepare the ground for the
inclusion of proper `Var` nodes in a minor release.
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: circuit Related to the core of the `QuantumCircuit` class or the circuit library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add manual variable support to expr.Var
4 participants