quantumlib / Cirq

A python framework for creating, editing, and invoking Noisy Intermediate Scale Quantum (NISQ) circuits.
Apache License 2.0
4.19k stars 995 forks source link

Support _circuit_diagram_info_protocol_ for customized rendering of tags in circuit diagrams #6560

Open pavoljuhas opened 2 months ago

pavoljuhas commented 2 months ago

This is a follow-up to #6411 which introduced str() as a default representation of tags in a text circuit diagrams. This should be kept as a default method, but we would like to also support the _circuit_diagram_info_protocol_ for tags. _circuit_diagram_info_protocol_ - if defined - would produce a specialized string for rendering the tag object in circuit diagrams.

What is the urgency from your perspective for this issue? Is it blocking important work?

P2 - we should do it in the next couple of quarters

xXxH3LIOSxXx commented 2 months ago

@richrines1 @maffoo - FYI - this was the spin off from maffoo's comment

We talked about it on the sync today briefly. My curiosity/concerns were:

SarthakNikhal commented 2 months ago

Can I help with this issue

xXxH3LIOSxXx commented 2 months ago

My thoughts from the past few weeks

What we covered in today's sync

Grain of salt on these 3 points, these are my own personal interpretation of the feedback obtained during the call. @pavoljuhas - feel free to correct any potential misunderstandings on my part on those summary points!

My current proposal for a way to proceed

Looking forward to anyone's thoughts on these points!!

xXxH3LIOSxXx commented 2 months ago

Can I help with this issue

./wave @SarthakNikhal - I think the door's open to anyone (if I'm any indication of that :) ) - and I would love a partner in crime on this deeper-than-it-first-appeared concept! Could you have a read over my last post and let me know what you think? If it makes sense to meet to discuss rather than bloat the issue dialogue lets figure out a way to do that.

@pavoljuhas - In cases where we want to collaborate on this - is the best practice to keep all the dialogue/thoughts here or to try to limit or consolidate updates here? I wasn't sure if my last lengthy post was really desirable or not or if I want to spitball with Sarthak, if that's really welcomed here on the issue itself.

pavoljuhas commented 2 months ago

@xXxH3LIOSxXx - thank you for a detailed summary of discussions thus far.

I feel the best way to go around this is to start with a minimum useful extension and leave more intricate use cases for later when (and if) they prove necessary. As it is the CircuitDiagramInfoArgs class which customizes the rendering of diagrams has include_tags attribute which can be used to show or ignore tag presence.

I think the easiest option is to adjust the TaggedOperation._circuit_diagram_info_ method to check if the present tag objects provide _circuit_diagram_info_ themselves.
If so, the method should return tag rendering in wire_symbols[0] in a similar fashion as done for LineQubit. For tags that do not have _circuit_diagram_info_ the rendering should fall back to str(tag) as done now.

git grep 'class \S*Tag\b' "*py" ":(exclude)*test.py" shows 5 non-test SomeTag classes. I feel for now we should leave their rendering as is, but the test-only Tag classes can and should exercise the new capability for the circuit_diagram_info protocol.

@xXxH3LIOSxXx - since you commented first and already showed interest in #6411 - can you take this on at this minimum initial scope?

Thank you BTW both (@SarthakNikhal) for your offer to help!

xXxH3LIOSxXx commented 2 months ago

@pavoljuhas - Got it and excited to begin digesting this and working on it!