Fix ne for self-loops #2
Conversation
|
@SimonDanisch Would you please review this PR? Thank you. |
|
Uh, I don't know this package :D |
|
@SimonDanisch Sorry for tagging you. Thank you for pinging the right person. |
src/simpleweightedgraph.jl
Outdated
| @@ -18,7 +18,7 @@ mutable struct SimpleWeightedGraph{T<:Integer, U<:Real} <: AbstractSimpleWeighte | |||
|
|
|||
| end | |||
|
|
|||
| ne(g::SimpleWeightedGraph) = nnz(g.weights) ÷ 2 | |||
| ne(g::SimpleWeightedGraph) = (nnz(g.weights) + count(diag(g.weights) .!= 0)) ÷ 2 | |||
There was a problem hiding this comment.
I think the assumption is, that ne is kind of fast. Ideally we would solve this by either storing the number of edges or the number of self-loops in a separate field, but I guess for now we can also just query for the diagonal elements.
But diag does also allocate a new Vector, so I think we should at least some non-allocating function here.
There was a problem hiding this comment.
Maybe nselfloop helps?
|
Thanks for your changes. In the future we probably should also make a distinction between values that are zero and values that are not in the structure of the sparse matrix, but for now I will just merge this and tag a new patch version, in order to make packages depending on this work. |
This feature should be fixed first. Execution performance could have room to discuss.