Visualize boxes with disjoint vertical spans in parallel #14386
Conversation
|
One or more of the following people are relevant to this code:
|
|
@ihincks A little bit of history. When we were developing the visualizations for ControlFlowOps, we ran into trouble when there were gates above and below the CFOs. Ordinary gates are placed on a grid and gate widths are created in increments of a basic gate size. CFOs, because of their complex nature, nesting, and various internal text uses only use the grid for the left-hand placement. If we hadn't done that, the CFOs would have been much larger with a great deal of empty space inside. Each nested CFO has its own grid for the gates inside it. What this means is that the 'condition' line going down, as in your ex. 2, may appear anywhere horizontally, including through a gate below. In addition, there is a whole section of code and data that figures out how to place the non-CFO gates when the qubit lines wrap to a new set and this code does not work if gates are above or below a CFO. So the decision was made to do the good enough solution with the thought that the wrapping and condition line problems might be handled later. Bottom line is that your 'span' solution probably wouldn't work with a complex circuit with gates above and below the BoxOps. (I'm just assuming this from memory, but haven't actually tried your solution). |
There was a problem hiding this comment.
Thanks for the change Ian, certainly from the examples I think this is a big improvement, and I don't immediately remember any of the considerations around line-splitting that would cause the box drawing like this to be a problem.
There's one text-visualisation test that needs a fix in the code, and I suspect that there might be reference-image tests in tests/visual that fail, but for those, it's typically easiest to push up a commit, let the CI run, then download the test artefact, which includes the generated images, and add those to the PR. I don't think we need to add new tests, because the existing ones seem to be covering it already, we just have to update the source of truth.
Can you add a one-sentence release-note?
|
Thanks for the history Ed - you've got a better memory than I do about that. I guess let's try merging this and then fix the bugs when we find them again - the new |
Pull Request Test Coverage Report for Build 15076434238Details
💛 - Coveralls |
|
Thanks for the background info @enavarro51 ! I'll keep my eye out for regressions as I plan to use this feature quite a bit over the coming months. |
* Change the qubit span of boxes to not be all qubits * update comment * better docstring * fix broken test * update reference plot * reno
Summary
This PR excepts BoxOp from the logic that causes ControlFlowOps to render sequentially in visualization.
Details and comments
The rational is that boxes don't share other concerns like having multiple blocks or direct classical dependencies (though their contents might). However, I can't pretend to understand completely the implications of this change given the complexity of the visualization folder that I am new to. More than anything, I haven't been able to find examples where this exception causes a problem, but it's easy to find examples that look way better with this change.
A couple of illustrative examples.
1.
WITH this PR:
WITHOUT this PR:
2.
WITH this PR:
WITHOUT this PR:
3.
WITH this PR:
WITHOUT this PR:
