LightGraphs.jl icon indicating copy to clipboard operation
LightGraphs.jl copied to clipboard

fix merge_vertices

Open etiennedeg opened this issue 3 years ago • 1 comments

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 on g, 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 Trait IsDirected, as it is done in reverse)?
  • From when is it worth to specialize methods for AbstractSimpleGraphs? Also, it seems that a lot of methods defined for AbstractGraph use the assumption that vertices form a continuous range (for example a_star, if I'm not mistaking, as it uses vertices as indices of a Vector).

etiennedeg avatar Sep 02 '21 22:09 etiennedeg

Codecov Report

Merging #1590 (9990ce3) into master (712b8d1) will decrease coverage by 0.00%. The diff coverage is 100.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              

codecov[bot] avatar Sep 21 '21 20:09 codecov[bot]