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

New SimpleGraph constructor from tuples #252

Closed
wants to merge 1 commit into from

Conversation

GiggleLiu
Copy link
Contributor

@GiggleLiu GiggleLiu commented May 2, 2023

From the slack discussion. We already have the
SimpleGraphFromIterator(vector_of_edges) https://juliagraphs.org/Graphs.jl/dev/first_steps/construction/#Modifying-graphs (@gdalle )

However, I think this new interface might still be very useful

  1. It does not require converting tuples to the Edge type.
  2. Allows setting the number of vertices, which I think is very important when constructing graphs programmingly.

In mathematics, we also use tuple of length 2 to represent a pair of undirected edges $(i, j) \in E$. To me, it is natural notation. Comments are welcomes.

Example

Before

g = SimpleGraph(Int32(4))

for (i, j) in  [(1, 2), (2, 3)]
    add_edge!(g, i, j)
end

After

g = SimpleGraph(Int32(4), [(1, 2), (2, 3)])

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #252 (75d96a1) into master (a10ca67) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #252   +/-   ##
=======================================
  Coverage   97.23%   97.23%           
=======================================
  Files         114      114           
  Lines        6583     6589    +6     
=======================================
+ Hits         6401     6407    +6     
  Misses        182      182           

@simonschoelly
Copy link
Member

Thanks for this contribution. In the past we had tried to not extend the number of constructors any further, in order not to grow the already large list of constructors any further, but maybe it it would be useful if instead of having to write

SimpleGraph(Edge.([
  (1,2),
  (3,4),
]))

we can simply write

SimpleGraph([
  (1,2),
  (3,4),
])

Maybe we can simplify the documentation by having both the same docstring for both constructors, so that it shows up as only one when checking the documentation.

That being said, I think we should stay consistent, and implement SimpleGraph(list_of_tuples) instead of SimpeGraph(n, list_of_tuples), as we have done it similarly for the vector of edges. In the rare case that one wants more isolated vertices, they can always call add_vertices! afterwards, with not much more computational cost.

I guess we might also think about implementing SimpleGraph{T}(list_of_tuples) so that one can specify the eltype there and won't have to do that for each tuple - in that case we should also do that for the edge list constructors. I am not sure why we did not already do that in the past.

Also, if we implement this new constructor, we should also do the same for SimpleDiGraph, SimpleGraphFromIterator and SimpleDiGraphFromIterator.

@gdalle as you were involved in the discussion on slack, you might also have an opinion on this.

@GiggleLiu
Copy link
Contributor Author

GiggleLiu commented May 7, 2023

Maybe we can simplify the documentation by having both the same docstring for both constructors, so that it shows up as only one when checking the documentation.

Agreed.

implement SimpleGraph(list_of_tuples) instead of SimpeGraph(n, list_of_tuples)

I knew the first interface long ago, but I never use it for productivity. Maybe also due to my using cases are often related to graph gadget finding that dangling vertices are allowed. I think allowing users to only remember only one API is important. The first interface may cover a lot of using cases, but I still need to remember multiple ways to construct the graph.

Consider the following using case: In a program, one wants to cast an adjacency matrix to a SimpleGraph. The first constructor may give you incorrect number of vertices. With the new interface, we can write

julia> SimpleGraph(10, getfield.(findall(!iszero, adj), :I))

I guess we might also think about implementing SimpleGraph{T}(list_of_tuples) so that one can specify the eltype there and won't have to do that for each tuple - in that case we should also do that for the edge list constructors. I am not sure why we did not already do that in the past.

In the added constructor, the data type follows the first argument, the number of vertices n.

Also, if we implement this new constructor, we should also do the same for SimpleDiGraph, SimpleGraphFromIterator and SimpleDiGraphFromIterator.

Sure, I can do that.

@gdalle gdalle added the enhancement New feature or request label Jun 16, 2023
@gdalle
Copy link
Member

gdalle commented Sep 14, 2023

I'm not sure about the tuple syntax, but I agree with the need for the number of vertices to feature as an argument

@gdalle
Copy link
Member

gdalle commented Jan 29, 2024

On second thought the list of constructors is already quite large, and adding Edge.(...) is only a minor inconvenience. I would suggest we close this, if other JuliaGraphs members agree. Thank you nonetheless for your contribution @GiggleLiu

@GiggleLiu
Copy link
Contributor Author

GiggleLiu commented Jan 29, 2024

@gdalle Please feel free to close this PR. This PR needs a lot extra effort to keep other similar interfaces consistent. Unfortunately, I have not had time to check other interfaces yet, sorry about that.

@gdalle
Copy link
Member

gdalle commented Jan 29, 2024

No worries, lack of workforce is also the reason behind my reasoning that simpler interfaces are easier to maintain! But please don't feel discouraged to open other issues or PRs in the future

@gdalle gdalle closed this Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants