Conversation
This reverts commit b3c00760cac82c6826c807383b87f7bb75d0691a.
I have been messing up with the branches in my pull requests, trying to solve this now
Codecov Report
@@ Coverage Diff @@
## master #28 +/- ##
=======================================
Coverage 99.45% 99.46%
=======================================
Files 106 107 +1
Lines 5554 5562 +8
=======================================
+ Hits 5524 5532 +8
Misses 30 30 |
| """ | ||
|
|
||
| function s_metric(g::AbstractGraph{T}; norm=true) where T | ||
| s = zero(T) |
There was a problem hiding this comment.
Maybe zero(T) is not the right type here. For small graphs it might be something like Int8 that would overflow quite soon. Maybe it is best, to use Int here, or Float64, then the resulting type would be the same no matter if norm is set to true or false.
| ``` | ||
| """ | ||
|
|
||
| function s_metric(g::AbstractGraph{T}; norm=true) where T |
There was a problem hiding this comment.
| function s_metric(g::AbstractGraph{T}; norm=true) where T | |
| function s_metric(g::AbstractGraph{T}; norm::Bool=true) where T |
| function s_metric(g::AbstractGraph{T}; norm=true) where T | ||
| s = zero(T) | ||
| for e in edges(g) | ||
| s += outdegree(g, src(e)) * indegree(g, dst(e)) |
There was a problem hiding this comment.
Do we need some special handling for self-loops here?
| sm2 /= sum(degree(g).^3)/2 | ||
| @test @inferred sm ≈ sm2 | ||
| end | ||
| end |
There was a problem hiding this comment.
Can we have some tests here for graphs different from random graphs? I am especially thinking about graphs with self-loops and isolated vertices.
Also, it would be good to test with graphs of eltype different than Int.
| @test @inferred sm ≈ sm2 | ||
| sm = s_metric(g, norm=true) |
There was a problem hiding this comment.
| @test @inferred sm ≈ sm2 | |
| sm = s_metric(g, norm=true) | |
| @test sm ≈ sm2 | |
| sm = @inferred s_metric(g, norm=true) |
The inferred macro is used to check if a function is type stable, i.e. the compiler can infer its resulting argument. What you are testing here is, if ≈ is type stable, which is probably not what you want.
|
@etiennedeg @vboussange This PR already looks quite good, but I think we need to fix a few things before we can merge it. |
|
This has bitrotted a bit, with a bunch of merge conflicts. I will mark it as a draft to keep my review queue more organized, but do not hesitate to convert it back to not a draft if you have the energy to push it forward. |
5 participants
This is a port of #1548