Conversation
| Return the bipartite subgraph of `g` induced by the disjoint subsets of vertices X and Y. | ||
|
|
||
| """ | ||
| function induced_bipartite_subgraph(g::T,X::AbstractVector{U},Y::AbstractVector{U}) where T <: AbstractGraph where U <: Integer |
There was a problem hiding this comment.
From convention it would be better to use G instead of T for the graph type. Furthermore, as far as I understand, this method would not work on any kind of AbstractGraph, only on SimpleGraph and SimpleDiGraph. So I would restrict it to these types of graphs.
There was a problem hiding this comment.
I copied the style of induced_subgraph that also uses T for the graph type. If this is more consistent with the convention, I can change it. Also, we could follow what's been done for induced_subgraph which also returns a mapping from the subgraph to the whole graph. This solution would let people use this function for any AbstractGraph (for example, using the mapping to retrieve information in a metagraph)
There was a problem hiding this comment.
I think following induced_subgraph is a good idea for the mapping, for the types I'd go with G
| g10 = complete_graph(10) | ||
| @testset "Induced bipartite Subgraphs: $g" for g in testgraphs(g10) | ||
| sg = @inferred(induced_bipartite_subgraph(g, [2,3],[4,5])) | ||
| @test nv(sg) == 4 | ||
| @test ne(sg) == 4 | ||
| @test is_bipartite(sg) | ||
| end |
There was a problem hiding this comment.
While this is good, I think we need a few more test cases, e.g.
- For empty graphs
- For graphs with self-loops
- For graphs that are not complete graphs
- For directed graphs
- For cases, where an exception is being thrown
- For
Uof different type thanInt
It might also be good to test more than just some properties, namely if the subgraph is actually the one that we wanted.
There was a problem hiding this comment.
I agree that more tests would be nice
|
Thanks for your PR :) I added a few comments. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #147 +/- ##
==========================================
+ Coverage 97.54% 98.19% +0.64%
==========================================
Files 109 109
Lines 6314 5757 -557
==========================================
- Hits 6159 5653 -506
+ Misses 155 104 -51 🚀 New features to boost your workflow:
|
| Return the bipartite subgraph of `g` induced by the disjoint subsets of vertices X and Y. | ||
|
|
||
| """ | ||
| function induced_bipartite_subgraph(g::T,X::AbstractVector{U},Y::AbstractVector{U}) where T <: AbstractGraph where U <: Integer |
There was a problem hiding this comment.
I think following induced_subgraph is a good idea for the mapping, for the types I'd go with G
| """ | ||
| function induced_bipartite_subgraph(g::T,X::AbstractVector{U},Y::AbstractVector{U}) where T <: AbstractGraph where U <: Integer | ||
|
|
||
| X ∩ Y != [] && throw(ArgumentError("X and Y sould not intersect!")) |
There was a problem hiding this comment.
would this ever be satisfied with an untyped array? better to test the length
|
|
||
|
|
||
| n = length(X) + length(Y) | ||
| G = T(n) |
There was a problem hiding this comment.
no guarantee this constructor exists (I'm not sure how we solve that though)
| @test sort(vm) == [1:5;] | ||
| end | ||
|
|
||
|
|
| g10 = complete_graph(10) | ||
| @testset "Induced bipartite Subgraphs: $g" for g in testgraphs(g10) | ||
| sg = @inferred(induced_bipartite_subgraph(g, [2,3],[4,5])) | ||
| @test nv(sg) == 4 | ||
| @test ne(sg) == 4 | ||
| @test is_bipartite(sg) | ||
| end |
There was a problem hiding this comment.
I agree that more tests would be nice
3 participants
Added support for induced bipartite subgraphs as discussed in this issue.