-
Notifications
You must be signed in to change notification settings - Fork 6
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
Replace CFG diagram with mermaid. #239
Conversation
Come to think of it this also applies if all successors want the same inputs. Maybe it is just a sum of null types in that case. Do we implicitly drop null inputs to a DFG? |
So the first Output in a basic block (i.e., the first input to the Output node) is a Sum type, each of whose elements is a Tuple. If there is one successor, that's a 1-ary Sum of some Tuple. The elements of the tuple are prepended onto the other (non-Sum) Outputs. So, a boolean is a Sum of two Tuples, each of which is a 0-ary tuple i.e. I really like your I've not used mermaid, but I'd be quite happy take the opportunity to experiment with it a bit and try to write an alternative (into another branch, perhaps). But perhaps also you can see where you'd like to go from here! |
Thanks, that makes perfect sense, I was having a braino, forgetting that it was a sum of tuples and an empty tuple unpacks to nothing at all. Thanks for pointing out the redundant wiring -- I'll fix that. The "call it |
Ok, sounds good. Also the "..." - I think this doesn't need to touch the other values (they can be wired directly into the Output node), it just needs to be a (load)Constant, producing the 0-bit value whose type is (1-ary sum of (0-ary tuple)) - one might say, |
Oh, I realized that in order to make this PR self-consistent I should also update the diagram below that is equivalent to this one. Will add that next. |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first of these looks good - the difference in the two P?
is perhaps a little confusing but basically fine. However, the second seems to be laid out terribly, to the point of being quite hard to understand. Screenshot below - I wonder if it's the same on your system, and/or whether there's anything we can do?
Let's remove attachments/2647818241/2647818458.png also.
specification/hugr.md
Outdated
control edges; the remaining outputs are those that are passed to all | ||
succeeding nodes. | ||
|
||
The three nodes labelled "..." are simply generating a predicate with one empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd label these "Const"
specification/hugr.md
Outdated
- If the entry node’s has only one successor and that successor is the | ||
exit node, the CFG-node itself can be removed | ||
The CFG in the example below has three inputs: one (call it `v`) of type "P" | ||
(not specified, but with a conversion to boolean represented by "P?"), one of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the P?
nodes in the graph are not doing the same thing - their types are different, as they are putting different values into the tuples (inside the sum). I'd either
- reword this to suggest that they are different (perhaps even rename - P1 and P2, or Query1 and Query2, say); or,
- use a simple bool type
[()|()]
everywhere, and therefore the same set of inputs (including both P and angle) to both BB1 and BB3, and then ideally note where the extra Inputs are being discarded. (Since we don't have explicit copy/discard anymore I'm not sure about the last, maybe it'd be ok to silently drop them).
specification/hugr.md
Outdated
subgraph BB1 | ||
direction TB | ||
BB1In["Input"] -- "angle" --> G | ||
BB1In -- "(Order)" --> BB1_["..."] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest BB1In -.-> BB1_["..."]
i.e. use dotted edges for Order....but then I see you are using them for nonlocal edges. Hmmm. There are not many edge styles available :-(. How about making the nonlocal edges thick to emphasize them, as that diagram is specifically in the section on nonlocal edges? :)
specification/hugr.md
Outdated
BB3 -- "0" --> Exit | ||
end | ||
A -- "P" --> CFG | ||
Q_pre["qubit source"] -- "qubit" --> Rz_out["Rz"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you combine A
, qubit_source
and angle_source
into just A
? That'd be more like the first diagram, and maybe it'd fit at the top then
BB1_ -- "[()]" --> BB1Out["Output"] | ||
G -- "angle" --> BB1Out | ||
end | ||
subgraph BB2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So most of the layout problem here looks to stem from mermaid putting BB1 as starting above BB2 (and adding a bunch of space inside BB1 at the top to make it high enough). I don't know why it does that, but I wonder if shuffling the order of BB1/BB2 might help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes no difference whatsoever, but let me see if there's anything else I can do to make it look more sensible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No joy, will make a github issue to revisit this.
Looks like one can also apply colours to edges -- I might do that to distinguish dataflow from controlflow edges. |
Ah yes. If you have three colours for controlflow, order, and dataflow, which sounds good in itself, then you could also go back to nonlocal being dotted. As you like! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behaviour of mermaid renderer with nonlocal edges is sufficiently weird that we probably can't fix it :-(. But LGTM otherwise :)
This replaces the first of the CFG diagrams.
It did raise one question for me: when a basic block (DFB node) has just one successor, the sum type in the predicate output should have arity 1. But there is nothing for it to contain, since all outputs would go in the other outputs (assuming we adopt the general policy of putting all common outputs there). Should we special-case this and omit the predicate output?