simonschoelly commented 3 years ago

Previously the == method was overriden for AbstractSimpleEdge but not hash. In order for dictionaries for edges to work correctly, we also have to implement hash so that e1 == e2 implies hash(e1) == hash(e2).

This has some serious consequences for MetaGraphs.jl, where weights(g) alawys returns values of 1.0 whenever the eltype of a MetaGraph is not Int. See the example below:

This PR should fix that, but I was wondering if we also should have some fix in MetaGraphs to ensure that no one produces incorrect results whenever they use a not up-to-date version of LightGraphs. Maybe we could just enforce a new enough version of LightGraphs in Project.toml.


julia> g = MetaGraph(squash(smallgraph(:house)))
{5, 6} undirected UInt8 metagraph with Float64 weights defined by :weight (default weight 1.0)

julia> set_prop!(g, 1, 2, :weight, 2.0)
true

julia> get_prop(g, 1, 2, :weight)
ERROR: KeyError: key :weight not found
Stacktrace:
 [1] getindex at ./dict.jl:467 [inlined]
 [2] get_prop at /home/simon/.julia/dev/MetaGraphs/src/MetaGraphs.jl:256 [inlined]
 [3] get_prop(::MetaGraph{UInt8,Float64}, ::Int64, ::Int64, ::Symbol) at /home/simon/.julia/dev/MetaGraphs/src/MetaGraphs.jl:258
 [4] top-level scope at REPL[4]:1

julia> weights(g)[1, 2]
1.0

codecov[bot] commented 3 years ago

Codecov Report

Merging #1514 (e5595b2) into master (ce53372) will increase coverage by 6.73%. The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #1514      +/-   ##
==========================================
+ Coverage   92.66%   99.39%   +6.73%     
==========================================
  Files         195      107      -88     
  Lines        6325     5156    -1169     
==========================================
- Hits         5861     5125     -736     
+ Misses        464       31     -433

matbesancon commented 3 years ago

if we add this to a patch version, we can hope folks will upgrade their LG

sbromberger commented 3 years ago

Maybe we could just enforce a new enough version of LightGraphs in Project.toml.

This is the most correct way of doing this. I'll commit this bugfix and then tag a new release. Could you please make a PR to MetaGraphs Project.toml to specify the new release? Then we can tag a new MetaGraphs release.

Also, generally speaking, should we implement hash on other edge types (like SimpleWeightedEdge)?

Thanks!

sbromberger / LightGraphs.jl

Implement hash for AbstractSimpleEdge #1514

Codecov Report