Skip to content
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

mermaid rendering child node in incorrect DataflowBlock #1197

Closed
ss2165 opened this issue Jun 17, 2024 · 1 comment · Fixed by CQCL/portgraph#139 or #1263
Closed

mermaid rendering child node in incorrect DataflowBlock #1197

ss2165 opened this issue Jun 17, 2024 · 1 comment · Fixed by CQCL/portgraph#139 or #1263
Assignees
Labels
bug Something isn't working

Comments

@ss2165
Copy link
Member

ss2165 commented Jun 17, 2024

When a Dom edge is connected to an output node, that node is being rendered inside the source dataflow block rather than the target.

Sample hugr:

{"version":"v1","nodes":[{"parent":0,"op":"CFG","signature":{"t":"G","input":[{"t":"Sum","s":"Unit","size":1}],"output":[],"extension_reqs":[]}},{"parent":0,"op":"DataflowBlock","inputs":[{"t":"Sum","s":"Unit","size":1}],"other_outputs":[],"sum_rows":[[]],"extension_delta":[]},{"parent":1,"op":"Input","types":[{"t":"Sum","s":"Unit","size":1}]},{"parent":1,"op":"Output","types":[{"t":"Sum","s":"Unit","size":1}]},{"parent":0,"op":"ExitBlock","cfg_outputs":[]},{"parent":0,"op":"DataflowBlock","inputs":[],"other_outputs":[],"sum_rows":[[]],"extension_delta":[]},{"parent":5,"op":"Input","types":[]},{"parent":5,"op":"Output","types":[{"t":"Sum","s":"Unit","size":1}]}],"edges":[[[2,0],[3,0]],[[2,0],[7,0]],[[1,0],[5,0]],[[5,0],[4,0]]],"metadata":null,"encoder":null}

(output node (7) correctly reports parent node DataflowBlock(5))

mermaid:

graph LR
    subgraph 0 ["(0) CFG"]
        direction LR
        subgraph 1 ["(1) DataflowBlock"]
            direction LR
            2["(2) Input"]
            2--"0:0<br>[]"-->3
            2--"0:0<br>[]"-->7
            3["(3) Output"]
        end
        1-."0:0".->5
        4["(4) ExitBlock"]
        subgraph 5 ["(5) DataflowBlock"]
            direction LR
            6["(6) Input"]
            7["(7) Output"]
        end
        5-."0:0".->4
    end
Loading

Node 7 is now being rendered inside block 1 even though though in the mermaid source it is inside subgraph 5. Maybe a mermaid bug?

Potential fix is adding an invisible edge between all input-output nodes to pin them to the same subgraph?

@ss2165 ss2165 added the bug Something isn't working label Jun 17, 2024
@aborgna-q
Copy link
Collaborator

aborgna-q commented Jun 17, 2024

I'm not sure it's a bug, the semantics are not too clear.

This can be fixed if write inter-graph edges outside the subgraph blocks.
The invisible edge may still help with layout.

graph LR
    subgraph 0 ["(0) CFG"]
        direction LR
        subgraph 1 ["(1) DataflowBlock"]
            direction LR
            2["(2) Input"]
            2--"0:0<br>[]"-->3
            3["(3) Output"]
        end
        1-."0:0".->5
        2--"0:0<br>[]"-->7
        4["(4) ExitBlock"]
        subgraph 5 ["(5) DataflowBlock"]
            direction LR
            6["(6) Input"]
            7["(7) Output"]
        end
        5-."0:0".->4
    end
Loading
graph LR
    subgraph 0 ["(0) CFG"]
        direction LR
        subgraph 1 ["(1) DataflowBlock"]
            direction LR
            2["(2) Input"]
            2--"0:0<br>[]"-->3
            3["(3) Output"]
        end
        1-."0:0".->5
        2--"0:0<br>[]"-->7
        4["(4) ExitBlock"]
        subgraph 5 ["(5) DataflowBlock"]
            direction LR
            6["(6) Input"]
            7["(7) Output"]
        end
        5-."0:0".->4
    end

github-merge-queue bot pushed a commit to CQCL/portgraph that referenced this issue Jun 24, 2024
Moves the tests for both to a common module, an uses `rstest::fixtures`
plus `insta` snapshots to test things.

Adds a `hierarchy_interregional` test case that shows the bug from
CQCL/hugr#1197.
@aborgna-q aborgna-q self-assigned this Jul 1, 2024
github-merge-queue bot pushed a commit to CQCL/portgraph that referenced this issue Jul 5, 2024
Defines intergraph edges on the parent region, so mermaid renders them
correctly.

This required a lowest-common-ancestor implementation for the hierarchy.
See #138.
I replaced the recursive DFS with a stack-based one that reuses the
structures when exploring multiple trees.

I also added some benchmarks for both rendering algorithms.

This closes CQCL/hugr#1197
github-merge-queue bot pushed a commit that referenced this issue Jul 5, 2024
aborgna-q added a commit that referenced this issue Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants