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 minimal support for standalone Var to visualisers #12307

Merged
merged 2 commits into from
May 1, 2024

Conversation

jakelishman
Copy link
Member

Summary

This adds best-effort only support to the visualisers for handling stand-alone Var nodes. Most of the changes are actually in qasm3, since the visualisers use internal details of that to handle the nodes.

This commit decouples the visualisers slightly more from the inner workings of the OQ3 exporter by having them manage their own variable-naming contexts and using the encapsulated _ExprBuilder, rather than poking into random internals of the full circuit exporter. This is necessary to allow the OQ3 exporter to expand to support these variables itself, and also for the visualisers, since variables may now be introduced in inner scopes.

This commit does not attempt to solve many of the known problems around zero-operand "gates", of which Store is one, just leaving it un-drawn. Printing to OpenQASM 3 is possibly a better visualisation strategy for large dynamic circuits for the time being.

Details and comments

Close #10937 (enough, at least, not to crash - it's not perfect by any stretch).

@enavarro51: would you be able to / like to review the changes to the visualisers that I made?

This adds best-effort only support to the visualisers for handling
stand-alone `Var` nodes.  Most of the changes are actually in `qasm3`,
since the visualisers use internal details of that to handle the nodes.

This commit decouples the visualisers _slightly_ more from the inner
workings of the OQ3 exporter by having them manage their own
variable-naming contexts and using the encapsulated `_ExprBuilder`,
rather than poking into random internals of the full circuit exporter.
This is necessary to allow the OQ3 exporter to expand to support these
variables itself, and also for the visualisers, since variables may now
be introduced in inner scopes.

This commit does not attempt to solve many of the known problems around
zero-operand "gates", of which `Store` is one, just leaving it un-drawn.
Printing to OpenQASM 3 is possibly a better visualisation strategy for
large dynamic circuits for the time being.
@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog mod: visualization qiskit.visualization mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Apr 29, 2024
@jakelishman jakelishman added this to the 1.1.0 milestone Apr 29, 2024
@jakelishman jakelishman requested review from nonhermitian and a team as code owners April 29, 2024 22:53
@qiskit-bot
Copy link
Collaborator

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

@coveralls
Copy link

coveralls commented Apr 29, 2024

Pull Request Test Coverage Report for Build 8915026681

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

  • 31 of 61 (50.82%) changed or added relevant lines in 5 files are covered.
  • 413 unchanged lines in 20 files lost coverage.
  • Overall coverage increased (+0.09%) to 89.539%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/qasm3/ast.py 5 6 83.33%
qiskit/visualization/circuit/text.py 19 22 86.36%
qiskit/qasm3/printer.py 2 6 33.33%
qiskit/qasm3/exporter.py 2 8 25.0%
qiskit/visualization/circuit/matplotlib.py 3 19 15.79%
Files with Coverage Reduction New Missed Lines %
qiskit/converters/circuit_to_gate.py 1 97.44%
qiskit/visualization/circuit/matplotlib.py 1 49.33%
qiskit/circuit/controlflow/control_flow.py 1 92.86%
qiskit/converters/circuit_to_instruction.py 1 98.28%
qiskit/circuit/library/blueprintcircuit.py 3 95.16%
qiskit/circuit/library/generalized_gates/isometry.py 3 94.59%
qiskit/providers/basic_provider/basic_provider_tools.py 5 86.49%
qiskit/circuit/_classical_resource_map.py 6 90.67%
crates/qasm2/src/lex.rs 6 92.62%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 8882717215: 0.09%
Covered Lines: 61599
Relevant Lines: 68796

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

Besides a couple of typos this looks fine to me. I'm not a huge fan how coupled the visualizations and the oq3 exported code is though. It sort of reminds me of a light version of the oq2 parser and dagcircuit integration. But that's outside the scope of this PR.

qiskit/qasm3/ast.py Outdated Show resolved Hide resolved
qiskit/visualization/circuit/matplotlib.py Outdated Show resolved Hide resolved
Co-authored-by: Matthew Treinish <[email protected]>
@jakelishman
Copy link
Member Author

Yeah, the coupling in the expr renderers isn't ideal, but hopefully this PR decouples them rather more. There's still a single private-class access, but that's also a component that's deliberately encapsulated away from the rest of the OQ3 exporter.

The coupling was a bit of a compromise - we don't have the resources to maintain the drawers as much as we'd like, and expression unparsing/formatting/whatever is not entirely trivial when it comes to determining when brackets are necessary in nested expressions. The OQ3 exporter already has to do that, so in the choice between duplicating additional code into the visualisers that'd need to be maintained and having a not-entirely-ideal coupling between the two, we tried out the coupling.

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.

I'm fine with this as is. Code wise this looks fine and it unblocks the other PRs on the stack for var support. The piece that I think is missing from the visualizers functionally is the input and store side. Looking at the output visualizations just showing the variables in the expressions is potentially confusing because you don't know where a or b come from or what the values or types are. But also at a certain point I don't think graphical visualization makes sense anymore and we'll likely just want to dump qasm or something instead to reason about the contents of a circuit in a "human" readable way. So this is not a regression in anyway and it's better than crashing or actively erroring with a var in an expression. We can follow up here with more improvements later.

@mtreinish mtreinish enabled auto-merge May 1, 2024 21:22
@mtreinish mtreinish added this pull request to the merge queue May 1, 2024
Merged via the queue into Qiskit:main with commit a78c941 May 1, 2024
15 checks passed
@jakelishman jakelishman deleted the var/visual branch May 1, 2024 23:34
ElePT pushed a commit to ElePT/qiskit that referenced this pull request May 31, 2024
* Add minimal support for standalone `Var` to visualisers

This adds best-effort only support to the visualisers for handling
stand-alone `Var` nodes.  Most of the changes are actually in `qasm3`,
since the visualisers use internal details of that to handle the nodes.

This commit decouples the visualisers _slightly_ more from the inner
workings of the OQ3 exporter by having them manage their own
variable-naming contexts and using the encapsulated `_ExprBuilder`,
rather than poking into random internals of the full circuit exporter.
This is necessary to allow the OQ3 exporter to expand to support these
variables itself, and also for the visualisers, since variables may now
be introduced in inner scopes.

This commit does not attempt to solve many of the known problems around
zero-operand "gates", of which `Store` is one, just leaving it un-drawn.
Printing to OpenQASM 3 is possibly a better visualisation strategy for
large dynamic circuits for the time being.

* Fix typos

Co-authored-by: Matthew Treinish <[email protected]>

---------

Co-authored-by: Matthew Treinish <[email protected]>
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 mod: visualization qiskit.visualization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for visualising classical variables
4 participants