Closed jsenellart closed 2 months ago
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?
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.
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:
_add_shape
=> calling add_shape
primitives in the canvas renderers_update_mode_style
Let us know if you can figure this out !
(see @raksharuia for further help)
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.
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.
Would that be the expected output?
I found two other issues in the rendering code:
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.
Hello @burlemarxiste ,
The graphics you show look nice. That's exactly what's expected.
About the issues you've found in the code:
recursive
might not be the best parameter naming, you're right.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.You can check my code here.
https://github.com/Quandela/Perceval/compare/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:
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.IDENTITY
and BARRIER
sound a bit loud, but other unitary ops are uppercase.@
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.
I'll check your code soon.
perceval.Circuit
. Why did you need to implement it?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).@
operator of yours :)Thanks for your answer on the pre-renderer.
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
.
Separate branch with only the renderer fix.
https://github.com/Quandela/Perceval/compare/main...burlemarxiste:Perceval:fix_heralds
Thanks I'll review it and test it tomorrow.
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 :)
Describe the bug
Perceval has a renderer at the
Circuit
or theProcessor
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
or
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)