Add SiblingMut allowing modification of a SiblingGraph #522

acl-cqc · 2023-09-12T10:50:47Z

Combine HugrView::get_io methods - could pull that out into a preliminary PR, it's just tidying a wrapper function
Also HugrMut::add_op_with_parent is just a wrapper around add_node_with_parent so make that a default-impl too
SibingMut implementation parallels SiblingView for many methods, but not all, as does not its own PortGraph
Reimplement OutlineCfg to use nested SiblingMut instances (escaping to &mut Hugr for set_parent)
Refactor tests so we test calling OutlineCfg on a SiblingMut of only a portion of a Hugr

src/hugr/rewrite/outline_cfg.rs

src/hugr/hugrmut.rs

src/hugr/rewrite/outline_cfg.rs

src/hugr/views/sibling.rs

…th valid_node checks ==> SiblingGraph::new's "get_optype" will implicitly check the root is in the base view. This does mean some unnecessary index checking for &Hugr's. We could continue to reimplement those methods for AsRef<Hugr> if we want to avoid this inefficiency. Alternatively we could make UnmanagedDenseMap return Options rather than unwrapping. Some other ugly alternative/hacks for returning default values - DEFAULT_{NODE,OP}TYPE

…e_hugr in c'tor

…aseless

…EMPTY_GRAPH

…iling without??

acl-cqc · 2023-09-14T09:59:03Z

Ok have merged with #535 which makes things quite a bit simpler. @agustín Borgna think this is ready now!

acl-cqc · 2023-09-14T10:00:49Z

src/hugr/hugrmut.rs

@@ -37,8 +37,8 @@ pub trait HugrMut: HugrView + HugrMutInternals {
        parent: Node,
        op: impl Into<OpType>,
    ) -> Result<Node, HugrError> {
-        self.valid_node(parent)?;
-        self.hugr_mut().add_op_with_parent(parent, op)
+        // TODO: Default to `NodeType::open_extensions` once we can infer extensions


(This comment just moved from the other add_op_with_parent)

acl-cqc · 2023-09-14T10:01:35Z

src/hugr/rewrite/outline_cfg.rs

+        };
+
+        // 4(b). Reconnect exit edge to the new exit node within the inner CFG
+        let mut in_bb_view: SiblingMut<'_, BasicBlockID> =


I guess maybe I should comment why we're building these SiblingMut's (i.e. in case the HugrMut we're operating on is a SiblingMut itself), as they would not be necessary if the h: impl HugrMut was in fact a &mut Hugr

aborgna-q

Nice!

aborgna-q · 2023-09-14T10:44:50Z

src/hugr/views/sibling.rs

+
+    fn linked_ports(&self, node: Node, port: Port) -> Self::PortLinks<'_> {
+        match self.contains_node(node) {
+            true => self.base_hugr().linked_ports(node, port),


This won't filter external edges.

aborgna-q · 2023-09-14T10:45:16Z

src/hugr/views/sibling.rs

+
+    fn node_connections(&self, node: Node, other: Node) -> Self::NodeConnections<'_> {
+        match self.contains_node(node) && self.contains_node(other) {
+            true => self.base_hugr().node_connections(node, other),


Same here, this isn't filtering the nodes.

aborgna-q · 2023-09-14T10:47:10Z

src/hugr/views/sibling.rs

+        self.valid_node(node).unwrap(); // ?? Or return empty iterator?
+        self.base_hugr().neighbours(node, dir) // Or self.hugr ?
+    }
+
+    fn all_neighbours(&self, node: Node) -> Self::Neighbours<'_> {
+        self.valid_node(node).unwrap(); // ?? Or return empty iterator?
+        self.base_hugr().all_neighbours(node)


I'd say return an empty one.

Ooops, missed those when I fixed a bunch of other unwraps. Done, thanks! :)

aborgna-q · 2023-09-14T10:51:46Z

src/hugr/views/sibling.rs

I wonder if delegate!ing all the HugrView impl to a SiblingGraph is feasible. In theory it should be free after all the layers are optimized away...
(there is no caching nor anything on a flat filtered graph, we only do that for descendants).

Have you tried that?

Not seriously. The only two strategies I had thought about before, are easily dismissed:

Keep a SiblingGraph permanently in the SiblingMut. But that means there's an immutable borrow of the underlying Hugr sitting around, which means we can't mutate it, which is pretty useless.

Construct a SiblingGraph on demand, call the method, then discard it. This is plausible for things like 'get_parentbut falls down for any of the iterators - we need (e.g.)SiblingMut.nodesto return thenodes()of the SiblingGraph, and the nodes refers to the SiblingGraph, which is a local variable deallocated as soon asSiblingMut.nodes` returns. (Returning a reference to temporary variable). So that doesn't fly.

However, ok, thinking a bit more....

what I'd like to do is declare a struct SGHolder<T> which holds (owns) a SiblingGraph, and a T which can refer to that SiblingGraph; and then impl...Iterator for SGHolder<T> where T: Iterator. However, it doesn't seem there's any way to have a struct like that - e.g. here or many others.

I think the only option remaining is to change the nodes() method of HugrView itself to return not a Self::Nodes<'_> but a Self::Nodes<'t> for some lifetime 't longer than Self - this might have to be a parameter of the nodes() method and we might need a max constraint from the other direction as part of HugrView (i.e. trait HugrView<'g> { .... fn<'t> nodes(&self) -> Self::Nodes<'t> where 't: '_, 'g: 't - I think). Then nodes() can return an iterator that references the Hugr inside the view, but not the view itself. However this may lead to other problems... (shout if you want but otherwise I'll see where I get to.)

No, the latter would work if SiblingGraph methods were returning things that referenced the underlying Hugr, but not the portgraph, as that's part of SiblingGraph (and gets deallocated when the SiblingMut methods return)...

So we'd have to store a portgraph in the SiblingMut? I.e. keep it up-to-date following each mutation (unless we change HugrView methods to take &mut self). I don't think we want to do that, but thoughts?

I also see suggestions that we change other classes (=~= the various iterators) to use Arc/Rcs rather than &s, but given we want to prevent mutation of the SiblingMut while the iterators are live, I don't see how that works.

I'll look at the various refs-between-fields-of-a-struct crates linked above, tho I think these are basically wrappers over unsafe....

Ok, ouroboros allows the intra-field-refs OK, but still seems to struggle with our fancy lifetime-parameterized trait members (like type Nodes<'a>) - see branch sibling-mut/ouroboros (commit 95cfb0d, failing with a "lifetime doesn't live long enough" error - seems to me like '1 and '2 in that error should both be '_ i.e. the lifetime of the enclosing SiblingMut??). Starting to feel like I'm out of ideas here...

So I tried parameterizing the HugrView and using Ouroboros there, to add a nodes2() that returns an iterator outliving the view itself...but still got stuck. I think the fundamental issue is that with Ouroboros, you define a struct holding a Foo and a Bar that "borrows" the Foo; but when you create the struct, you provide a function to create the Bar given an &Foo but the types we are using require the Bar returned by that function to outlive the &Foo reference (passed to the function). Possibly we could get around this by going right the way into portgraph and adding ouroboros-type magic there, but I don't think I'm going there.

Fundamentally, we want some way to convert, say, a portgraph::Children<'a> into a Children<'static>, i.e. copying the extra data.

But I think there is a much easier way to do that - copy the iterated items, rather than the data over which the iterator runs. I'll give that a go here....

…h and collect vec into iter

acl-cqc · 2023-09-15T13:00:33Z

Ok, so

have fixed the missing filters, where SiblingGraph uses Portgraph, and the dodgy unwraps, by delegating to a temporarily-constructed SiblingGraph and then copying the result through a Vec. This does mean we temporarily construct a portgraph and throw it away (which I think we kind of have to) but also do another pass of copying, so might not be the absolute most efficient way, but the alternative(s) seem much nastier (invasive use of crate ouroborus in portgraph?? unsafe blocks??) and/or just require a lot of reimplementation (i.e. SiblingGraph + SiblingMut implementing the same functionality but in two totally different ways). So I think we can return to this if it turns out to be inefficient - this way at least keeps things simple and uncoupled/modular.
commoned up remaining methods that are "Same as SiblingGraph" via a `base_members" macro. Coupling aside (!), it's super-easy (no parameters) so hopefully not too objectionable.

aborgna-q

Yeah, this is a non-trivial problem :P

This look alright as a first implementation. I say open a P-low perf issue to investigate the performance here.

aborgna-q · 2023-09-15T13:07:19Z

src/hugr/views/sibling.rs

@@ -41,24 +43,63 @@ pub struct SiblingGraph<'g, Root = Node> {
    _phantom: std::marker::PhantomData<Root>,
 }

+/// HugrView trait members common to both [SiblingGraph] and [SiblingMut],
+/// i.e. that rely only on [HugrInternals::base_hugr]
+macro_rules! base_members {


Uhm, this is iffy but I guess it's ok.
I'd just rename it to impl_base_members to convey a bit more info at the callsites.

acl-cqc commented Sep 12, 2023

View reviewed changes

src/hugr/rewrite/outline_cfg.rs Outdated Show resolved Hide resolved

acl-cqc commented Sep 12, 2023

View reviewed changes

src/hugr/hugrmut.rs Outdated Show resolved Hide resolved

acl-cqc commented Sep 12, 2023

View reviewed changes

src/hugr/rewrite/outline_cfg.rs Outdated Show resolved Hide resolved

acl-cqc commented Sep 12, 2023

View reviewed changes

src/hugr/views/sibling.rs Outdated Show resolved Hide resolved

Base automatically changed from new/hugrmut_apply to main September 12, 2023 12:20

acl-cqc added 6 commits September 12, 2023 15:59

Move valid_node/valid_non_root into HugrView (no uses yet)

e28c60a

Remove Base type-parameter to SiblingGraph/DescendantsGraph, call bas…

17c3a30

…e_hugr in c'tor

Derive Clone for SiblingGraph + DescendantsGraph

8475d12

HierarchyView::new returns Result/HugrError

aa88f75

test

487c795

acl-cqc marked this pull request as draft September 12, 2023 16:33

acl-cqc added 6 commits September 12, 2023 18:08

Merge remote-tracking branch 'origin/main' into new/hugr_view_valid_b…

ea70450

…aseless

Remove debug println

430e4e6

HugrMut::add_op_with_parent

e98562b

Add SiblingMut

f42f666

Preliminary: factor out build_conditional_in_loop from ..._cfg

2930a3b

New test (breaking out to base_hugr/hugr_mut when necessary).

b4b4e5e

acl-cqc force-pushed the new/sibling-mut branch from 054bac3 to b4b4e5e Compare September 12, 2023 17:59

fn depth: take &Hugr

f331bfd

acl-cqc mentioned this pull request Sep 12, 2023

HugrView: validate nodes, and remove Base #523

Merged

acl-cqc added 9 commits September 13, 2023 10:22

use NodeMetadata::Null

c789969

correct comment re. view/filter

6507bbc

new -> try_new

929ecf4

Merge remote-tracking branch 'origin/main' into new/hugr_view_valid_b…

663f47f

…aseless

Merge branch 'new/hugr_view_valid_baseless' into new/sibling-mut

1839b70

Add Root typaram, new->try_new (fixing doclink)

b37fbc7

Missed fmt

e702acb

HugrView::get_io is just a wrapper, need to implement only once

ee3892e

Implement HugrView methods to return empty iterators via lazy_static …

2c47e54

…EMPTY_GRAPH

acl-cqc marked this pull request as ready for review September 13, 2023 18:31

acl-cqc requested a review from aborgna-q September 13, 2023 18:31

acl-cqc added 2 commits September 14, 2023 08:18

Missing typaram for impl HugrMut on SiblingMut. How on earth was comp…

341a4e9

…iling without??

correct comments

55f2510

acl-cqc force-pushed the new/sibling-mut branch from 9f0af34 to 55f2510 Compare September 14, 2023 07:20

acl-cqc added 4 commits September 14, 2023 10:37

Merge remote-tracking branch 'origin/main' into HEAD

de48382

restructure w/nested SiblingMut's and break out to &mut Hugr less

f739eef

Remove import that clippy saw but couldn't remove

8848601

Comments, move reconnect

3f3a71d

acl-cqc changed the title ~~[WIP] Add SiblingMut allowing modification of a SiblingGraph~~ Add SiblingMut allowing modification of a SiblingGraph Sep 14, 2023

acl-cqc commented Sep 14, 2023

View reviewed changes

aborgna-q reviewed Sep 14, 2023

View reviewed changes

acl-cqc added 4 commits September 15, 2023 13:10

SiblingGraph::new_unchecked

eee3026

Properly filter, and don't unwrap, by constructing a temp SiblingGrap…

12c5a7c

…h and collect vec into iter

Another note about method being the same as SiblingGraph

ca35146

Common up via base_members macro

f0859c7

acl-cqc requested a review from aborgna-q September 15, 2023 13:00

aborgna-q approved these changes Sep 15, 2023

View reviewed changes

acl-cqc added 3 commits September 18, 2023 11:11

Remove unused EMPTY_GRAPH

b3124a3

comments

b67dba0

base_members => impl_base_members

dc4e1a2

acl-cqc added this pull request to the merge queue Sep 18, 2023

Merged via the queue into main with commit 0ea19dd Sep 18, 2023
6 checks passed

acl-cqc deleted the new/sibling-mut branch September 18, 2023 12:01

acl-cqc mentioned this pull request Sep 18, 2023

SiblingMut does lots of cloning and Portgraph-reconstruction #540

Open

acl-cqc mentioned this pull request Sep 27, 2023

Some way to apply a rewrite to a region of a Hugr #246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SiblingMut allowing modification of a SiblingGraph #522

Add SiblingMut allowing modification of a SiblingGraph #522

acl-cqc commented Sep 12, 2023 •

edited

Loading

acl-cqc commented Sep 14, 2023

acl-cqc Sep 14, 2023

acl-cqc Sep 14, 2023

aborgna-q left a comment

aborgna-q Sep 14, 2023

aborgna-q Sep 14, 2023

aborgna-q Sep 14, 2023

acl-cqc Sep 15, 2023

aborgna-q Sep 14, 2023

acl-cqc Sep 14, 2023 •

edited

Loading

acl-cqc Sep 15, 2023

acl-cqc Sep 15, 2023 •

edited

Loading

acl-cqc Sep 15, 2023

acl-cqc commented Sep 15, 2023

aborgna-q left a comment

aborgna-q Sep 15, 2023

Add SiblingMut allowing modification of a SiblingGraph #522

Add SiblingMut allowing modification of a SiblingGraph #522

Conversation

acl-cqc commented Sep 12, 2023 • edited Loading

acl-cqc commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aborgna-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Sep 15, 2023

aborgna-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Sep 12, 2023 •

edited

Loading

acl-cqc Sep 14, 2023 •

edited

Loading

acl-cqc Sep 15, 2023 •

edited

Loading