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

Squash does not work correctly for AbstractGraph #88

Closed
simonschoelly opened this issue Jan 9, 2022 · 3 comments
Closed

Squash does not work correctly for AbstractGraph #88

simonschoelly opened this issue Jan 9, 2022 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@simonschoelly
Copy link
Member

The way the squash is currently implemented is by either calling SimpleGraph{T}(g) or SimpleDiGraph{T}(g) for some integer type T. This fails if g is a subtype of AbstractGraph for which these functions are not implemented. There are multiple ways to solve this:

  1. Restrict squash to SimpleGraph and SimpleDiGraph and leave the implementation of that function for other graph types open.
  2. Ensure that the constructors SimpleGraph{T}(g) or SimpleDiGraph{T}(g) work for for any kind of g::AbstractGraph.
  3. Require that subtypes of any kind of graph type SomeGraph <: AbstractGraph implement a constructor SomeGraph{T}(g) and adjust squash accordingly.

In my opinion, 1. is the way to go. 2. could lead to some information (such as metadata) being thrown away by squash which is probably not what the user wants. 3. would make the interface more complicated and would make existing packages not correspond to the interface anymore.

@simonschoelly simonschoelly added the bug Something isn't working label Jan 9, 2022
@etiennedeg
Copy link
Member

I also think 1. is the way to go. It should nonetheless be possible to implement a conversion from AbstractGraph to SimpleGraph by collecting neighbors.

@simonschoelly
Copy link
Member Author

simonschoelly commented Jan 13, 2022

Yes, not with the squash function though. One way to do that is to implement functions

SimpleGraph(::AbstractGraph)
SimpleGraph{T}(::AbstractGraph)
SimpleDiGraph(::AbstractGraph)
SimpleDiGraph{T}(::AbstractGraph)

or with a different function name, to keep the number of constructs a bit low. We would also need to figure out what to do in cases when the argument is a directed graph. Currently there is already a constructor SimpleGraph(::SimpleDiGraph) which adds an undirected edge, if there is one edge in either direction in the directed graph.

@simonschoelly
Copy link
Member Author

Solved by #93

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants