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

Added strong_product, disjunctive_product, lexicographical_product, h… #154

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

dstahlke
Copy link

Added most of the graph products from https://en.wikipedia.org/wiki/Graph_product

test/operators.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Merging #154 (b7fab71) into master (897e183) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
+ Coverage   97.40%   97.43%   +0.03%     
==========================================
  Files         109      109              
  Lines        6470     6560      +90     
==========================================
+ Hits         6302     6392      +90     
  Misses        168      168              

@aurorarossi
Copy link
Member

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.
Thank you!

src/operators.jl Outdated
true
```
"""
function strong_product(g::G, h::G) where {G<:AbstractGraph}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

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

gdalle commented Jan 29, 2024

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 Simplegraphs. Furthermore, there is the question of types: what should a product of two GridGraphs be for instance? Fall back on a SimpleGraph type?

@dstahlke
Copy link
Author

dstahlke commented Feb 9, 2024

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?

@gdalle
Copy link
Member

gdalle commented Mar 5, 2024

I think restricting these new functions to AbstractSimpleGraph would make sense for now, but without touching the existing ones (cause that might be breaking)

@dstahlke
Copy link
Author

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.

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.

4 participants