LightGraphs.jl
LightGraphs.jl copied to clipboard
fix merge_vertices
Fix for 1586.
merge_vertices
was mutating vs
, this is fixed.
I also restricted the definition of the method from AbstractGraph
to AbstractSimpleGraph
, because it assumes the vertices are a OneTo range. Tell me if you think I should not change this method definition, but I think a more general method would be quite meaningless.
I harmonized it a bit with the in-place method and did some little optimizations. It is possible to do a little bit better by updating new_vertex_ids
only for vertices above merged_vertex
, should I do it, or is it ok like this? I hope I didn't break anything.
This is my first PR and I have a few questions :
- I see that some tests, notably for Operators.jl, are in a testset
"$g" for g in testlargegraphs(g3)
run on different types, but don't rely ong
, so these are run multiple times. Should we move these to another static testset ? I added a new test to cover the issue, and put it with these tests, but I can modify that. - Some functions (like
complement
) are defined for the concrete types Graph and DiGraph. Should these be implemented for AbstractSimpleGraph (with eventually the TraitIsDirected
, as it is done inreverse
)? - From when is it worth to specialize methods for
AbstractSimpleGraphs
? Also, it seems that a lot of methods defined forAbstractGraph
use the assumption that vertices form a continuous range (for examplea_star
, if I'm not mistaking, as it uses vertices as indices of aVector
).
Codecov Report
Merging #1590 (9990ce3) into master (712b8d1) will decrease coverage by
0.00%
. The diff coverage is100.00%
.
:exclamation: Current head 9990ce3 differs from pull request most recent head dd64cec. Consider uploading reports for the commit dd64cec to get more accurate results
@@ Coverage Diff @@
## master #1590 +/- ##
==========================================
- Coverage 99.44% 99.44% -0.01%
==========================================
Files 106 106
Lines 5551 5543 -8
==========================================
- Hits 5520 5512 -8
Misses 31 31