-
Notifications
You must be signed in to change notification settings - Fork 22
Use traits for (un)directed types #40
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
Conversation
|
Thanks! Using traits for this makes sense to me! |
|
I also agree with you about the |
|
If you want to help maintain I could add you as a collaborator? |
|
Do you have any comments on the PR ? Keep in mind that this is a breaking change, since I removed
I made an issue if more conversation needs to happen #42
Yes sure! I cannot promise working consistently but I will be of help, so please go on! |
|
What do you think @gdalle? |
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
==========================================
- Coverage 90.90% 90.83% -0.08%
==========================================
Files 8 7 -1
Lines 242 240 -2
==========================================
- Hits 220 218 -2
Misses 22 22
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Thanks for the great PR @filchristou, it's a really awesome idea! |
I find the definition of
MetaUndirectedGraphandMetaDiGraphvery constrained.Graphs.jlsolves this problem by using traits.Here, I made the changes so that the same solution is followed.
SimpleTraits.jlis already in the Manifest as an indirect dependency due to Graphs.jlWith this solution we can now build
MetaGraphs of any subtype ofAbstractGraphimplementingGraphs.is_directed()Before only
MetaGraphs ofSimpleGraphandSimpleDiGraphcould be built.Use case
For example, if this PR, together with QuantumBFS/Multigraphs.jl#18 and QuantumBFS/Multigraphs.jl#17 are completed, we will be able to have a
MetaGraphofMultigraph.This will be a huge advantage over MetaGraphs.jl, which for now it's also only limited to
SimpleGraphandSimpleDiGraph.More discussion
I don't quite agree with the
Graphs.SimpleGraphandGraphs.SimpleDiGraphmethods.To me they look like constructors and they should return a new object, and not the graph of the
MetaGraph.For this reason, we could instead have something simple like a
getgraphmethod.