feat: Add map_owned method for Graph#794
Conversation
There was a problem hiding this comment.
Hey, first off, thanks for the PR 🦕
Seems like a good addition, I just have a few notes :)
One thing I didn't include in the code review:
Is there any reason why this wouldn't be implemented for StableGraphs while we're at it?
Additionally, in the original issue it was suggested to add similar functionality for filter_map, so maybe add that as well?
| /// The resulting graph has the same structure and the same graph indices | ||
| /// as `self`. | ||
| /// | ||
| /// If you want a non-destructive version of this function, see `map`. |
There was a problem hiding this comment.
| /// If you want a non-destructive version of this function, see `map`. | |
| /// If you want a non-consuming version of this function, see | |
| /// [`map`](struct.Graph.html#method.map). |
I think, I would prefer to write non-consuming instead of destructive, since destruction might imply more than just giving up ownership of the value.
Also, we should reference the actual alternative map method.
| /// as `self`. | ||
| /// | ||
| /// If you want a non-destructive version of this function, see `map`. | ||
| pub fn map_owned<F, G, N2, E2>(self, mut node_map: F, mut edge_map: G) -> Graph<N2, E2, Ty, Ix> |
There was a problem hiding this comment.
Possible instead of map_owned we could call this method into_map aligning with naming conventions from std, such as into_iter or into_bytes. On the other hand, we could also go for map_into which is also used in itertools for example:
https://github.com/rust-itertools/itertools/blob/b0942d6b84d47dec75c19e541859211423d857d9/src/adaptors/map.rs#L125
map_into would make sense since we don't actually convert into a map (into an interator) but instead convert to a graph immediately (similar to applying collect to an iterator in the iterator analogy). Therefore we 'map' 'into' another graph. into_map on the other hand would suggest that we would return a map of some sort and consume the provided graph.
| #[test] | ||
| fn map_map_owned() { | ||
| let mut g = Graph::new_undirected(); | ||
| let a = g.add_node("A"); | ||
| let b = g.add_node("B"); | ||
| let c = g.add_node("C"); | ||
| let ab = g.add_edge(a, b, 7); | ||
| let bc = g.add_edge(b, c, 14); | ||
| let ca = g.add_edge(c, a, 9); | ||
|
|
||
| let g2 = g.map_owned(|_, name| format!("map-{name}"), |_, weight| weight * 2); | ||
| assert_eq!(g2.node_count(), 3); | ||
| assert_eq!(g2.node_weight(a).map(|s| &**s), Some("map-A")); | ||
| assert_eq!(g2.node_weight(b).map(|s| &**s), Some("map-B")); | ||
| assert_eq!(g2.node_weight(c).map(|s| &**s), Some("map-C")); | ||
| assert_eq!(g2.edge_count(), 3); | ||
| assert_eq!(g2.edge_weight(ab), Some(&14)); | ||
| assert_eq!(g2.edge_weight(bc), Some(&28)); | ||
| assert_eq!(g2.edge_weight(ca), Some(&18)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Not necessarily your concern but I am just noticing that we don't have any (normal) map tests for graph. So maybe add one for that as well while we are at it?
|
I suggest also implement |
|
Referenced this in the marked PRs by accident, sry ^^ |
starovoid
added
C-feature-accepted
RaoulLuque
added
the
S-waiting-on-author
|
Since this PR seems stale, I was wondering if it would be alright if I would pick it up. That is, fork your branch and finish it such that your work gets merged : ) |
|
@RaoulLuque Hey mate sorry. Been really busy over the last...yeesh, over a month. I'd really appreciate you picking it up :) Thanks!! |
Hey, no worries at all :) Just wanted to make sure I'd ask in order not step on anyone's toes 🌞 Appreciate your quick response :) |
|
Closed in favor of #863. |
…raph` (#863) This PR continues the work done in PR #794. Thus, it resolves #542. It adds `map_owned` and `filter_map_owned` methods for both `Graph` and `StableGraph` which take ownership of the respective graphs and their respective associated data in comparison to the `map` and `filter_map` counterparts which only take `&` references. Appropriate tests for this have been added. If desired, I can also add quickchecks, but this did not seem necessary. --------- Co-authored-by: Pete Hayes <[email protected]>
…raph` (petgraph#863) This PR continues the work done in PR petgraph#794. Thus, it resolves petgraph#542. It adds `map_owned` and `filter_map_owned` methods for both `Graph` and `StableGraph` which take ownership of the respective graphs and their respective associated data in comparison to the `map` and `filter_map` counterparts which only take `&` references. Appropriate tests for this have been added. If desired, I can also add quickchecks, but this did not seem necessary. --------- Co-authored-by: Pete Hayes <[email protected]>
…raph` (petgraph#863) This PR continues the work done in PR petgraph#794. Thus, it resolves petgraph#542. It adds `map_owned` and `filter_map_owned` methods for both `Graph` and `StableGraph` which take ownership of the respective graphs and their respective associated data in comparison to the `map` and `filter_map` counterparts which only take `&` references. Appropriate tests for this have been added. If desired, I can also add quickchecks, but this did not seem necessary. --------- Co-authored-by: Pete Hayes <[email protected]>
…raph` (petgraph#863) This PR continues the work done in PR petgraph#794. Thus, it resolves petgraph#542. It adds `map_owned` and `filter_map_owned` methods for both `Graph` and `StableGraph` which take ownership of the respective graphs and their respective associated data in comparison to the `map` and `filter_map` counterparts which only take `&` references. Appropriate tests for this have been added. If desired, I can also add quickchecks, but this did not seem necessary. --------- Co-authored-by: Pete Hayes <[email protected]>
3 participants
Closes #542