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

SiblingSubgraph does not handle Copies at output boundary #518

Closed
lmondada opened this issue Sep 11, 2023 · 2 comments · Fixed by #585
Closed

SiblingSubgraph does not handle Copies at output boundary #518

lmondada opened this issue Sep 11, 2023 · 2 comments · Fixed by #585
Labels
bug Something isn't working

Comments

@lmondada
Copy link
Contributor

The following test

    #[test]
    fn edge_both_output_and_copy() {
        let one_bit = type_row![BOOL_T];
        let two_bit = type_row![BOOL_T, BOOL_T];

        let mut builder =
            DFGBuilder::new(FunctionType::new(one_bit.clone(), two_bit.clone())).unwrap();
        let inw = builder.input_wires().exactly_one().unwrap();
        let outw1 = builder.add_dataflow_op(not_op(), [inw]).unwrap().out_wire(0);
        let outw2 = builder
            .add_dataflow_op(and_op(), [inw, outw1])
            .unwrap()
            .outputs();
        let outw = [outw1]
            .into_iter()
            .chain(outw2);
        let h = builder.finish_hugr_with_outputs(outw, &EMPTY_REG).unwrap();
        let view = SiblingGraph::<DfgID>::new(&h, h.root());
        let subg = SiblingSubgraph::try_new_dataflow_subgraph(&view).unwrap();
        assert_eq!(subg.nodes().len(), 1);
    }

encodes the computation

graph TD
IN --> NOT
IN --> AND
NOT --> AND
NOT --> OUT
AND --> OUT
Loading

This currently fails with the error NotConvex as the output of the NOT is taken to be an output of the subgraph, even though one copy is actually still within the subgraph and used for the AND gate.

@lmondada lmondada added the bug Something isn't working label Sep 11, 2023
github-merge-queue bot pushed a commit that referenced this issue Sep 12, 2023
I have added a TODO on line 234 in `sibling_subgraph.rs`, I suspect
there is a bug here that I will look into now.

EDIT: I've opened #518 but I won't have time to fix it until next
sprint. This can be merged in independently of that bug.
@cqc-alec
Copy link
Collaborator

@lmondada I think the final assertion in that test should be

        assert_eq!(subg.nodes().len(), 2);

Am I right?

@lmondada
Copy link
Contributor Author

Yes, absolutely!

github-merge-queue bot pushed a commit that referenced this issue Oct 2, 2023
Fixes #518 .

I changed the `non_convex_subgraph` test because the graph it was
constructing seemed to meet the criteria for convexity, and replaced it
with another one which doesn't. But please check -- I was a little
confused by the definitions and may be missing something.
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
Development

Successfully merging a pull request may close this issue.

2 participants