Skip to content

Fix classical control diagram symbols and svg #7233

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

daxfohl
Copy link
Collaborator

@daxfohl daxfohl commented Apr 6, 2025

Fixes #5688. Fixes #5689.

A quick image search for circuit diagrams shows that the convention seems to be arrows for measurement, which I represent here in diagrams with V. I followed the suggestion from #5688 to use the quantum control symbol @ to represent classical control as well.

For SVG, now the circuit from the teleportation example renders in a more standard fashion, with double-lines for classical registers and measurements, and arrows representing classical data flow:
image

(previously looked like this, which is obviously misleading):
image

I avoided the suggestion from #5688 to represent measurement as X in diagrams. The rationale is that X implies a bit flip. However in Cirq, multiple measurements to the same register will be stored independently in an array. So, representing those as X, which implies a bit flip, would be a misrepresentation.

I'll leave it open to discussion on whether this is correct and/or better. It's a three-character change in the source and then a global find-and-replace to patch up the tests and docs, so it's easy to accommodate whatever y'all decide here.

@daxfohl daxfohl requested review from vtomole and a team as code owners April 6, 2025 19:21
@daxfohl daxfohl requested a review from fdmalone April 6, 2025 19:21
Copy link

codecov bot commented Apr 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.65%. Comparing base (02941bf) to head (eaa0367).
Report is 67 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7233   +/-   ##
=======================================
  Coverage   98.65%   98.65%           
=======================================
  Files        1106     1106           
  Lines       95869    95882   +13     
=======================================
+ Hits        94581    94594   +13     
  Misses       1288     1288           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mhucka mhucka added triage/discuss Needs decision / discussion, bring these up during Cirq Cynque area/visualization labels Apr 7, 2025
@mhucka
Copy link
Contributor

mhucka commented Apr 7, 2025

I tagged this to be discussed at the next Cirq Cynq.

A change like this strikes me as a breaking change. I see in the comments on #5688 this concerns was brought up a couple of years ago, but not resolved at the time.

@mhucka mhucka added the priority/after-1.5 Leave for after the Cirq 1.5 release label Apr 7, 2025
@daxfohl
Copy link
Collaborator Author

daxfohl commented Apr 7, 2025

Yeah I understand the concern. If it's decided that this is breaking, we should also probably revisit other issues involving presentation of circuit diagrams (e.g. #1245, #3597, #3612, #3913, etc), SVG (#5689), 3D view, Bloch sphere, and so on, and close anything else that would be considered breaking.

@daxfohl daxfohl marked this pull request as draft April 7, 2025 19:07
@github-actions github-actions bot added the size: M 50< lines changed <250 label Apr 15, 2025
@daxfohl daxfohl marked this pull request as ready for review April 19, 2025 18:19
@daxfohl daxfohl changed the title Fix classical control diagram symbols Fix classical control diagram symbols and svg May 3, 2025
@mhucka
Copy link
Contributor

mhucka commented May 14, 2025

Discussed in Cirq Cynq 2025-05-14: it's not clear if this is a breaking change. In general, the feeling is that improving the diagrams and making them more standard and readable would be a good thing. (A breaking change would be more of a concern for function signatures, data output, etc. Diagrams are more often used for human consumption.)

@mhucka mhucka added Ready for Re-Review For when reviewers take their time. and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels May 14, 2025
@daxfohl
Copy link
Collaborator Author

daxfohl commented May 15, 2025

An old comment from a previous TL states that diagrams are not intended to be used as inputs: #3597 (comment). Also the intermediate data structure "CircuitDiagramInfo" isn't used for anything but generating diagrams, so the change is unlikely to result in any production breakages. So, obviously biased, but I don't think I'd call it a breaking change.

One remaining concern is if other teams have unit test cases that check diagram output of their circuit. Ideally test cases shouldn't use them that way, but sometimes it's convenient and/or more didactic to put the circuit diagram in the test case rather than a list of operations. I know we do that a fair amount internally, but I have a harder time imagining consumers doing the same. Also this only affects classical controls, which is a fairly niche feature, and the fix can be done in two global find-and-replaces: s/=@=/=V= and s/=^=/=@= in any test files that have circuit diagrams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/visualization priority/after-1.5 Leave for after the Cirq 1.5 release Ready for Re-Review For when reviewers take their time. size: M 50< lines changed <250
Projects
None yet
2 participants