Fix method ambiguity with sort!(::AbstractVector, ..., ::NOOPSortAlg, ...)

[Liozou]

The recent upstream commit JuliaLang/julia#45330 introduced a new method for sort!. It results in a potential method ambiguity that Aqua.jl detected in the nightly CI of PeriodicGraphEmbeddings.jl (see here).

So here is a quick fix.

For reference, this sort! method was added in 383eeb3. It should be safe to reduce the type of its argument x to AbstractVector since you rarely want to sort! anything else in general, and I think this particular method is only ever called in the traverse_graph! function in the same file, hence on a Vector.

[simonschoelly]

Thanks! I am willing to merge this as it is, but maybe it would even make more sense, if we do not override sort! here at all, but rather solve it differently. Maybe event just with an if to check if we should run sort! at all.

[codecov]

Codecov Report

Merging #140 (705f287) into master (7152d54) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #140   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files         109      109           
  Lines        6314     6314           
=======================================
  Hits         6159     6159           
  Misses        155      155           

[Liozou]

Sure, the smallest diff would be:

diff --git a/src/Experimental/Traversals/bfs.jl b/src/Experimental/Traversals/bfs.jl
index fdea62f9..55c8e053 100644
--- a/src/Experimental/Traversals/bfs.jl
+++ b/src/Experimental/Traversals/bfs.jl
@@ -4,8 +4,6 @@ import Base:sort!
 struct NOOPSortAlg <: Base.Sort.Algorithm end
 const NOOPSort = NOOPSortAlg()
 
-sort!(x::AbstractVector, ::Integer, ::Integer, ::NOOPSortAlg, ::Base.Sort.Ordering) = x
-
 struct BFS{T<:Base.Sort.Algorithm} <: TraversalAlgorithm
     sort_alg::T
 end
@@ -56,7 +54,7 @@ function traverse_graph!(
         postlevelfn!(state) || return false
         empty!(cur_level)
         cur_level, next_level = next_level, cur_level
-        sort!(cur_level, alg=alg.sort_alg)
+        alg.sort_alg isa NOOPSortAlg || sort!(cur_level, alg=alg.sort_alg)
     end
     return true
 end
diff --git a/test/experimental/traversals.jl b/test/experimental/traversals.jl
index d35e31b6..8d8408cc 100644
--- a/test/experimental/traversals.jl
+++ b/test/experimental/traversals.jl
@@ -11,9 +11,6 @@ struct DummyTraversalState <: LET.AbstractTraversalState end
     @test LET.postvisitfn!(d, 1) == true
     @test LET.postlevelfn!(d) == true
 
-    x = [3,1,2]
-    @test sort!(x, 3, 1, LET.NOOPSort, Base.Sort.ForwardOrdering()) == x
-
     @testset "BFS" begin
         g1= smallgraph(:house)
         dg1 = path_digraph(6); add_edge!(dg1, 2, 6)

But I think the multiple-dispatch way is less brittle, in case one wants to write generic code relying on this BFS type. I have to agree that it's a risky idea though, considering that everything here is marked as experimental; at the same time, this method is explicitly tested... So I don't have a very strong preference either.

Adding a new method to sort! might also cause some invalidations I suppose... But I don't think it applies in this case.

[etiennedeg]

This failure is not usual, any idea if it is related to this PR?

SBM: Test Failed at /home/runner/work/Graphs.jl/Graphs.jl/test/simplegraphs/generators/randgraphs.jl:287
  Expression: norm(collect(ratios)) < 0.25
   Evaluated: 36.601605475428485 < 0.25

[Liozou]

I just checked by adding a print statement in the sort! method: nothing pops up when testing Graphs/test/simplegraphs/runtests.jl (and it does print when testing Graphs/test/experimental/traversals.jl) so it should be unrelated. All Graphs.jl tests pass locally by the way, and I am latest julia master at bd8dbc388c7b89f68838ca554ed7ba91740cce75 which should be exactly the nightly build.

[etiennedeg]

Yes, it's probably due to the randomness of the test. I was intrigued at first by the huge gap, but this may be due to a very low denominator in a fraction.

[Liozou]

It looks like so, I also just checked on julia v1.6.6-pre.0 (close enough to the failing CI build) on ubuntu: everything passes locally.

[etiennedeg]

Yeah, I ran the test a lot of times, and it seems to fail nearly 1/1000 times, so not linked to this PR.

[Liozou]

@simonschoelly I don't mind switching to the diff I proposed above but do you prefer it?
Otherwise, don't hesitate to let me know if there is anything else I can do.

[Liozou]

Bump. This is now causing the nightly CI failures here.

[simonschoelly]

@Liozou in the meantime someone made the exact same PR and already merged it: #159 . So I am closing this, although I still think, we should not override sort!.

[matbesancon]

sorry I hadn't seen this PR before! IMO this is fine to overwrite since we introduce a specific other algorithm, a strange one though