Should undirected edges == and hash be undirected?

[DylanModesitt]

Edges for SimpleGraph still consider their directedness when testing for equality and hashing. This is understandable given that SimpleEdge is used for directed graphs also. Is it worth considering adding a SimpleUndirectedEdge (or similar)? Something along the lines of:

...

function ==(e1::SimpleUndirectedEdge, e2::SimpleUndirectedEdge)
    return (src(e1) == src(e2) && dst(e1) == dst(e2)) ||
        (src(e1) == dst(e2) && dst(e1) == src(e2))
end

hash(e::SimpleUndirectedEdge, h::UInt) = hash(src(e), h) ⊻ hash(dst(e), h)

And then having SimpleGraph instead return edges of this type by default?

This would more easily allow users to maintain sets / dictionaries of edges. For instance, the reason I'm opening this is the associated awkwardness I encountered when implementing a minimum cycle basis that maintains such edge sets.

Happy to do a PR if this change is palatable.

[gdalle]

Ran into a related issue recently for a graph that had both directed and undirected edges. At the moment this is not supported by Graphs.jl but maybe with something along the lines what you propose it could be

[gdalle]

@DylanModesitt a PR would be welcome but I don't know whether it would break things. @simonschoelly thoughts?

[simonschoelly]

I am also afraid this might break things - perhaps this is something we can do in a next mayor release. For simplicity I would propose the types

Edge  # undirected edge
DiEdge  # directed edge

or maybe UEdge instead of Edge?

Something I did in SimpleValueGraphs.jl was to enforce in the constructor of that type, that the first vertex is always smaller than the second one. Then we don't need to check in hash and equals.

[gdalle]

Yeah the presence of directed edges in undirected graph is a major correctness footgun in my opinion. I like the idea of ordering by default, and I think deprecating src / dst to replace them with neighbor(e, u) would be a welcome change