feat: add Subgraph::copy_in_parent #182
Conversation
This reverts commit 7e132b7.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 83.27% 83.54% +0.27%
==========================================
Files 24 24
Lines 6117 6316 +199
Branches 6117 6316 +199
==========================================
+ Hits 5094 5277 +183
- Misses 947 967 +20
+ Partials 76 72 -4 ☔ View full report in Codecov by Sentry. |
acl-cqc
changed the title
acl-cqc
changed the title
There was a problem hiding this comment.
Thanks, looks good to me, other than one small bug I think.
I'd rather get @aborgna-q's opinion on implementing the portgraph traits for references, however. I don't know all the consequences of adding the blanket trait impls. Are we fine with barring users from definining their own impls on references?
src/view/subgraph.rs
Outdated
| /// Returns a map from node indices within this subgraph, to the indices | ||
| /// of the newly-created nodes in the parent graph (they are not in this subgraph!); | ||
| /// or a LinkError in which case the underlying graph will not have had any nodes added | ||
| /// (however subports may have been for a [MultiView]). |
There was a problem hiding this comment.
It might be cleanest for the API if you would differentiate between the "expected" error of finding a boundary edge in a non-Multiview graph, vs the "seriously wrong error" of a link failing to get added for inscrutable reasons (and in which case it is hard to know whether other ports have already been added).
In the first case you could return a better error than a LinkError... But this might be unnecessarily complicating things.
There was a problem hiding this comment.
I think that's fair, it did make me think "ugh, hmmm" a bit while I was writing it. Lemme have a go...
There was a problem hiding this comment.
Hmmm, so the first thing I note is that any LinkError for internal edges must be a bug in copy_in_parent - perhaps we should unwrap that rather than return to the user. Then we can say any LinkError must be from the boundary edges. Moreover, the only possibility is AlreadyLinked if we are copying in a plain portgraph - we could panic/unwrap on any other variant as again that must be a bug in copy_in_parent. It would be nice thus to define an error type that for MultiPortgraphs is Infallible but for plain Portgraphs is only the "AlreadyLinked" variant of LinkError...
This may be too aggressive, it's not really the style of the rest of portgraph, @aborgna-q may have opinions here...
There was a problem hiding this comment.
-
I would remove the subports-added on failure note. Those are implementation details and not visible to the user unless they peel the multiport layer off.
-
Unwrap (or rather
expect) on internal bugs seems reasonable. Errors should be for user bugs. -
Use the standard rustdoc headers to clean this up a bit
/// # Returns
///
/// A map from node indices within this subgraph, to the indices
/// of the newly-created nodes in the parent graph (they are not in this subgraph!).
///
/// # Errors
///
/// - [CopySubgraphError::CantCopyBoundary] if the boundary was not empty, but graph doesn't support support multilinks. The underlying graph remains unchanged.
There was a problem hiding this comment.
Good plan, thanks - and that lead to spotting that I hadn't re-exported CopySubgraphError :). I kept a note that capacity may have changed rather than subports.
|
@aborgna-q given Luca has requested your review, shall I skip re-requesting Luca's ? |
| @@ -63,4 +107,32 @@ impl<G: MultiView> MultiView for &G { | |||
| } | |||
| } | |||
|
|
|||
| // TODO: PortMut, LinkMut, MultiMut | |||
There was a problem hiding this comment.
Could you add MultiMut too?
There was a problem hiding this comment.
Done, also MultiView for &mut G
src/view/subgraph.rs
Outdated
| impl<G: LinkMut> Subgraph<G> | ||
| where | ||
| G::LinkEndpoint: Into<PortIndex>, | ||
| { |
There was a problem hiding this comment.
This is already a bound on LinkEndpoint
Line 283 in 0748032
| impl<G: LinkMut> Subgraph<G> | |
| where | |
| G::LinkEndpoint: Into<PortIndex>, | |
| { | |
| impl<G: LinkMut> Subgraph<G> { |
There was a problem hiding this comment.
And I thought I was so clever for thinking of that!! :-D
src/view/subgraph.rs
Outdated
| /// Copies all the nodes and edges in this subgraph into the parent graph. | ||
| /// If there are any boundary edges, these will also be copied but keeping | ||
| /// the same *external* end port (this will fail unless the underlying graph | ||
| /// is a [MultiView]). |
There was a problem hiding this comment.
| /// is a [MultiView]). | |
| /// is a [`MultiMut`]). |
| where | ||
| G::LinkEndpoint: Into<PortIndex>, | ||
| { | ||
| /// Copies all the nodes and edges in this subgraph into the parent graph. |
There was a problem hiding this comment.
The first line is used as a short description on module listings.
| /// Copies all the nodes and edges in this subgraph into the parent graph. | |
| /// Copies all the nodes and edges in this subgraph into the parent graph. | |
| /// |
src/view/subgraph.rs
Outdated
| /// Returns a map from node indices within this subgraph, to the indices | ||
| /// of the newly-created nodes in the parent graph (they are not in this subgraph!); | ||
| /// or a LinkError in which case the underlying graph will not have had any nodes added | ||
| /// (however subports may have been for a [MultiView]). |
There was a problem hiding this comment.
-
I would remove the subports-added on failure note. Those are implementation details and not visible to the user unless they peel the multiport layer off.
-
Unwrap (or rather
expect) on internal bugs seems reasonable. Errors should be for user bugs. -
Use the standard rustdoc headers to clean this up a bit
/// # Returns
///
/// A map from node indices within this subgraph, to the indices
/// of the newly-created nodes in the parent graph (they are not in this subgraph!).
///
/// # Errors
///
/// - [CopySubgraphError::CantCopyBoundary] if the boundary was not empty, but graph doesn't support support multilinks. The underlying graph remains unchanged.
Bencher Report
|
I've raised #186 for benchmarking as I'd be interested to do it but think this is blocking too many things ATM. |
closes #171.
As suggested there, using Subgraph to define the, erm, subgraph to copy seemed good.
: Clonebounds, since&muts do not implement CloneThe API seems slightly strange in that the Subgraph defines what to copy, but of course then does not include the copy of that just made, but I think that makes sense.
Note that
Subgraphnever implementsLinkMutso we avoid the complex case of the parent itself being a Subgraph :)