-
Notifications
You must be signed in to change notification settings - Fork 94
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
=======================================
Coverage 97.23% 97.23%
=======================================
Files 114 114
Lines 6583 6589 +6
=======================================
+ Hits 6401 6407 +6
Misses 182 182 |
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 I guess we might also think about implementing Also, if we implement this new constructor, we should also do the same for @gdalle as you were involved in the discussion on slack, you might also have an opinion on this. |
Agreed.
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 julia> SimpleGraph(10, getfield.(findall(!iszero, adj), :I))
In the added constructor, the data type follows the first argument, the number of vertices
Sure, I can do that. |
I'm not sure about the tuple syntax, but I agree with the need for the number of vertices to feature as an argument |
On second thought the list of constructors is already quite large, and adding |
@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. |
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 |
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
Edge
type.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
After