Retain for stable graph

[Rupsbant]

Lets try this again.

[bluss]

Hi, to be frank with you here I'd expect the PR to explain the thoughts behind the change.

I disagree with adding a method that does not have the same signature as the corresponding one on Graph.

[Rupsbant]

It's my first public pull-request so I might do some things the wrong way. As for actual content:

I see your point. And considering this signature corresponds to the std::vec::Vec signature this should probably be reverted. I'll remove the output vector.

if you're curious for my thoughts:
First off, consider a non-clone node type. You can move them in the graph-node and when you remove them you get them back. If you don't use them they get dropped. However if you want to remove a lot at the same time. They either get destroyed or you need to remove them one-by-one (which is quadratic since you don't have an unsafe NodeIndex iterator.

I'd consider the Vec and Graph signature insufficient. However this needs to be measured against other concerns (which I did not do!):

• What happens if the allocation is not needed, can this be optimized away?
• How consistent is this signature?
• What is the cost of working around it? (Fixed by Rc or Option + take)

[bluss]

StableGraph and Graph should have the same interface where it's possible. It may be insufficient but having the interfaces mismatch only means it's a halfway done API transition: one or the other will need to change to match, otherwise the library is a mess. There is no trade off here IMO; it's about fixing the signature or starting to plan the name and signature of the new API.

From your message it sounds like there is no pressing use case behind the choice to return a Vec, and then I think the choice of design is simple (don't return the Vec).

(We have the benefit that removing nodes in sequence is not quadratic with the stable graph, since nodes don't shift. Regular Graph can achieve this too by removing them in reverse order, which is how Graph::retain_nodes is implemented.)

[Rupsbant]

To be sure: the tests fail because of an update to nightly. I think this is a compiler bug regarding scope. I opened an issue rust-lang/rust#40235

use petgraph::graph::node_index as n;
...
#[test]
fn filtered_post_order() {
    ...
    let mut dfs = DfsPostOrder::new(&gr, n(0));
    let f = NodeFiltered(&gr, map);
    while let Some(n) = dfs.next(&f) {
        po.push(n);
    }
    assert!(!po.contains(&n(1)));
}

[bluss]

Ah this should check first if ix is a node in the graph.

[bluss]

Oh and the upper bound is not node_count but the upper bound count.

[Rupsbant]

The node count is correct: the count of the unstable subgraph is used.
Added the check. The users should indeed not do this manually.

[bluss]

Thank you! I'm sorry for the delay. I'll try out the squash merge button, then I don't have to ask you to do the squash.. See you on the other side..

[bluss]

Looks like it worked!