Added strong_product, disjunctive_product, lexicographical_product, h…#154
Added strong_product, disjunctive_product, lexicographical_product, h…#154Krastanov merged 13 commits intoJuliaGraphs:masterfrom
Conversation
…omomorphic_product
Co-authored-by: Aurora Rossi <65721467+aurorarossi@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #154 +/- ##
==========================================
+ Coverage 97.22% 97.25% +0.03%
==========================================
Files 122 122
Lines 7240 7330 +90
==========================================
+ Hits 7039 7129 +90
Misses 201 201 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi! We recently added a JuliaFormatter and this pull request was made earlier, so it was not aligned. I fixed this problem and now the controls are fine except for one, since the homomorphic product is not tested. It would be great if you could provide one. |
| true | ||
| ``` | ||
| """ | ||
| function strong_product(g::G, h::G) where {G<:AbstractGraph} |
There was a problem hiding this comment.
Does this operator also work for directed graphs? And does it really make sense to define it for all graph types? Maybe we should restrict it to SimpleGraph and maybe also SimpleDiGraph.
Otherwise it might be a bit confusing if one has, for example, a graph with metadata on its vertices.
There was a problem hiding this comment.
Though I don't see mention in wikipedia of directed graphs in relation to any of these products, I think most of them would make sense. The only exception would be the homomorphic product which has (h_1 \nsim h_2) in its definition and there could be controversy over how that should be interpreted for directed graphs. Also this one is pretty obscure and I suspect nobody would ever want to use it on a directed graph.
Good point on not supporting graphs that have metadata. What would be the appropriate way to support both SimpleGraph and SimpleDiGraph? Should I use the common parent class AbstractSimpleGraph? (Note that the existing functions tensor_product and cartesian_product also have this issue.)
|
Making a round of unresolved PR conversations Metadata is not supported anywhere in Graphs.jl, that is why I don't think it's a requirement for graph products. The real issue is that these products require adding edges, and we can't guarantee this is supported beyond |
|
Is there anything needed from me? Should I make it take only SimpleGraph arguments rather than AbstractGraph? And if so, should I make the already existing graph product functions do the same? |
|
I think restricting these new functions to |
|
I changed SimpleGraph to AbstractSimpleGraph and fixed the doctests. The doctests had some trivial problems due to lines copied from tensor_product and somehow must not have been running before. |
|
I have been sorting through stalled and bitrotten PRs over the last hour or two, and I am really pleased that this one is actually in great condition and already reviewed in detail. I gave it a quick final pass as well. I will go ahead and merge it when all tests pass. Thank you for the contribution (and the immense patience). By the way, you would be unsurprised to hear that we are struggling to find enough volunteers to help with reviews. If you want to sing up as a volunteer, do not hesitate to join in reviewing -- it would be extremely valuable for the ecosystem. |
Added most of the graph products from https://en.wikipedia.org/wiki/Graph_product