Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

fix merge_vertices #1590

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

etiennedeg
Copy link

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).

@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #1590 (9990ce3) into master (712b8d1) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ 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              

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants