Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issymmetric(::SimpleDiGraph) not correct #248

Closed
jlapeyre opened this issue Apr 17, 2023 · 5 comments · Fixed by #265
Closed

issymmetric(::SimpleDiGraph) not correct #248

jlapeyre opened this issue Apr 17, 2023 · 5 comments · Fixed by #265
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@jlapeyre
Copy link
Contributor

jlapeyre commented Apr 17, 2023

This dispatches here

issymmetric(g::AbstractGraph) = !is_directed(g)

This of course returns false for any digraph, and digraphs can indeed be symmetric.

I don't know how many meanings of symmetric are common in fields using graphs. But wikipedia says this means that for each edge $(v, w)$ there is an edge $(w, v)$.

@simonschoelly
Copy link
Contributor

I agree with you here. I think if we change it, we have to do it, so that it is still true for all undirected graphs. For directed graphs I am not sure if there is an easy way to that so have some default for all directed AbstractGraph or if we simply should return a not-implemented message.

@simonschoelly simonschoelly added bug Something isn't working good first issue Good for newcomers labels May 18, 2023
@gdalle gdalle added this to the v1.9 milestone Jun 28, 2023
@gdalle gdalle mentioned this issue Jun 28, 2023
@gdalle gdalle linked a pull request Jun 28, 2023 that will close this issue
@etiennedeg
Copy link
Member

Should it also take into account weights or metadata ?

@gdalle
Copy link
Member

gdalle commented Jun 28, 2023

Should it also take into account weights or metadata ?

Weight maybe, metadata we can't do it anyway in the current state of affairs

@gdalle
Copy link
Member

gdalle commented Jun 28, 2023

But Graphs.jl itself doesn't have a method for getting the weight of an edge. Best we could do is to add an optional argument weights passing a matrix? But who would ever use this function anyway?

@simonschoelly
Copy link
Contributor

Should it also take into account weights or metadata ?

I think the solution that we currently have in #248 kind of solves this, although certainly there are other ways to handle that. It basically uses the edges(g), reverse(e), has_edge(g, e), and e1 == e2 functions to verify if a graph is symmetric. If some graph type has some weights/edge-metadata and correctly implements these functions, then symmetry is correctly checked.

Of course one could argue, that there should be a way to ignore weights/metadata but I am not sure how important that is. perhaps in the future, we can specify that with an additional parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants