Heralds out of their box

[jsenellart]

Describe the bug

Perceval has a renderer at the Circuit or the Processor level allowing to generate svg or png through matplotlib library. At the processor level, it is possible to add heralds on some modes - as a result, these modes are not anymore available and the heralds are showing inside the circuit.
However, the rendering of the circuit does not take these heralds into account which makes sometime the herald appears as overlapping with the circuit box.

To Reproduce
within Pycharm or any editor supporting in-app matplot display, or within a Jupyter notebook

import Perceval as pcvl
from perceval.components.core_catalog import PostProcessedCnotItem
pcvl.pdisplay(PostProcessedCnotItem().build(), recursive=True)

or

circuit = pcvl.Circuit(3).add(0, pcvl.BS()).add(1, pcvl.BS(1))
processor = pcvl.Processor('SLOS', circuit)
processor.add_herald(0,0)
pcvl.pdisplay(processor, recursive=True)

Expected behavior
The herald on the first mode should be displayed inside the circuit bounding box

Additional context
In notebook, a svg renderer is used (perceval/rendering/canvas/svg_canvas.py), in editor, matplotlib renderer is used (perceval/rendering/canvas/mplot_canvas.py)

[alt-shreya]

I'd like to work on this. I have reproduced the error on a Jupyter notebook. What would be a good starting point to go about solving this?

[alt-shreya]

I've inspected the code, and I suspect there might be some slight miscalculation in the precompute_herald_pos function. Checking implementation of PERM to see what I can find there.

[jsenellart]

Hello @alt-shreya, thanks for your interest in the issue 👍 ! I believe that the core issue is coming from the fact the heralds are added after the circuit, we would expect them to "push" a bit the top beamsplitter while the circuit is being rendered.

However how it is done currently:

• the circuit is drawn using _add_shape => calling add_shape primitives in the canvas renderers
• the heralds are added (so too late to influence the circuit display) using _update_mode_style

Let us know if you can figure this out !

(see @raksharuia for further help)

[king-p3nguin]

Hi @alt-shreya, are you still working on this issue? I'm also interested in this issue but I do not want to disturb you if you are about to figure out.

[alt-shreya]

hi @king-p3nguin, thank you for checking in. I have been working on it, yes. In all honesty, I haven't figured out the final solution yet, but I'm hoping to take another crack at it.

[burlemarxiste]

Would that be the expected output?

I found two other issues in the rendering code:

• Occasionally, heralds were not moved inside their subblock and stayed on the right. I had to refactor the code gathering herald information prior to diagram rendering.
• The recursive argument in render_circuit is not passed along, so there is never more than one level of nesting. Is that expected? I would suggest a recursion_level parameter to control the amount of nesting.

The fix that generated the diagrams above comes with a caveat: we need two passes over the object graph, one that mimicks the main rendering but just keeps track of potential overlaps between objects (implemented as a PreRenderer, the main rendering being informed by its output.

[ericbrts]

Hello @burlemarxiste ,

The graphics you show look nice. That's exactly what's expected.

About the issues you've found in the code:

• Ok, it might be a corner case as I'm not sure I've seen this bug before. Really cool that you fixed it nonetheless
• We made some tests, at the beginning of Perceval, showing all levels of recursion and it made the rendered circuit unreadable most of the time. We'd like to stay as it is. But then recursive might not be the best parameter naming, you're right.
• About the dual pass: if it multiplies all pdisplay calls by two, we need to assess if it's still usable. If the first pass is fast, that'll probably do the trick.

[burlemarxiste]

You can check my code here.

main...burlemarxiste:Perceval:main

It includes the implementation of barriers, simply because my initial way of approaching the rendering problem was to create a dummy circuit element that could serve as a spacer. This was a wrong approach, but half of the code for a barrier was done.

Things on which I'd appreciate feedback:

• Whether my implementation of IDENTITY is as innocuous as it seems for the rest of the ecosystem (eg, won't break or slow down a simulation). I tried to locate unit tests evaluating large circuits (eg Grover in test_polarization_simulator.py) and checked that adding barriers did not break anything. I don't think it's worth modifying this specific unit test just to verify that a barrier does not break anything.
• The names IDENTITY and BARRIER sound a bit loud, but other unitary ops are uppercase.
• The shortcut @ to add a barrier followed by another circuit element.

I tried to make flake8 as happy as possible with renderer.py, this has the side effect of "spamming" the diff with many formatting changes. But I noticed many other files in the codebase do not comply.

To answer your question about performance, the new implementation of pdisplay instantiates both the renderer and pre-renderer. The pre-renderer doesn't draw anything, it just keeps track of positions (updating _chart) and takes milliseconds to run.

[ericbrts]

I'll check your code soon.

• Identity is just an empty perceval.Circuit. Why did you need to implement it?
• Names are a bit loud yes. Except for PERM all are short acronyms. The only other is Unitary that follows PascalCase. So I'd say you should go for PascalCase for Barrier (and Identity if needed).
• I'll check this @ operator of yours :)

Thanks for your answer on the pre-renderer.

[burlemarxiste]

In order to implement the barriers, I needed an element that had no effect on the states but that could be put in a circuit, that has an x order, that is rendered... so an empty Circuit() couldn't do the trick.

Barrier is the version of it that is visually rendered as a barrier. But if you think it's clutter, I can directly implement the behavior of Identity into Barrier.

[burlemarxiste]

Separate branch with only the renderer fix.

main...burlemarxiste:Perceval:fix_heralds

[ericbrts]

Thanks I'll review it and test it tomorrow.

[ericbrts]

Hey @burlemarxiste , closing this issue now and you're assigned to it. You're the UnitaryHack 2024 bounty winner for this issue.
Thanks for your (2nd contribution) - waiting to complete the 3rd on the other PR :)