Conversation
…s were added in the wrong location (before the "Undirected Line Graph" tests instead of after). Let me correct that:
test/operators.jl
```julia
<<<<<<< SEARCH
@testset "Length: $(typeof(g))" for g in test_generic_graphs(SimpleGraph(100))
@test length(g) == 10000
end
end
@testset "Undirected Line Graph" begin
=======
@testset "Length: $(typeof(g))" for g in test_generic_graphs(SimpleGraph(100))
@test length(g) == 10000
end
end
@testset "Directed Line Graph" begin
@testset "Directed Cycle Graphs" begin
for n in 3:9
g = cycle_digraph(n)
lg = line_graph(g)
@test nv(lg) == n
@test ne(lg) == n
@test is_directed(lg)
@test all(outdegree(lg, v) == 1 for v in vertices(lg))
@test all(indegree(lg, v) == 1 for v in vertices(lg))
end
end
@testset "Directed Path Graphs" begin
for n in 2:9
g = path_digraph(n)
lg = line_graph(g)
@test nv(lg) == n - 1
@test ne(lg) == n - 2
@test is_directed(lg)
@test is_connected(lg)
@test all(outdegree(lg, v) <= 1 for v in vertices(lg))
@test all(indegree(lg, v) <= 1 for v in vertices(lg))
if n > 2
@test indegree(lg, 1) == 0
@test outdegree(lg, 1) == 1
@test indegree(lg, nv(lg)) == 1
@test outdegree(lg, nv(lg)) == 0
end
end
end
@testset "Directed Star Graphs" begin
for n in 3:9
g = star_digraph(n)
lg = line_graph(g)
@test nv(lg) == n - 1
@test ne(lg) == 0
end
for n in 3:9
g = SimpleDiGraph(n)
for i in 2:n
add_edge!(g, 1, i)
add_edge!(g, i, 1)
end
lg = line_graph(g)
@test nv(lg) == 2*(n-1)
@test ne(lg) == (n-1) + (n-1)*(n-1)
end
end
@testset "Directed Self-loops" begin
g = SimpleDiGraph(2)
add_edge!(g, 1, 1)
add_edge!(g, 1, 2)
lg = line_graph(g)
@test nv(lg) == 2
@test ne(lg) == 2
@test has_edge(lg, 1, 1)
@test has_edge(lg, 1, 2)
@test !has_edge(lg, 2, 1)
@test !has_edge(lg, 2, 2)
end
end
@testset "Undirected Line Graph" begin
>>>>>>> REPLACE
```
This change:
1. Adds the new directed line graph tests in the correct location (after the length tests and before the undirected line graph tests)
2. Maintains all existing tests exactly as they were
3. Adds comprehensive tests for directed line graphs with the same structure as the undirected tests
The tests cover:
- Directed cycle graphs (should produce directed cycles)
- Directed path graphs (should produce shorter directed paths)
- Directed star graphs (both unidirectional and bidirectional cases)
- Directed graphs with self-loops (testing edge cases)
Each test verifies the expected number of vertices, edges, and connectivity properties of the resulting line graph.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #440 +/- ##
==========================================
+ Coverage 97.41% 97.42% +0.01%
==========================================
Files 120 120
Lines 6959 7002 +43
==========================================
+ Hits 6779 6822 +43
Misses 180 180 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Krastanov There seems to be an issue with the pre-commit bot - is this the same issue that you mentioned in another PR? |
|
@simonschoelly I'd appreciate any feedback. Are the unit tests sufficient, should I add or remove some of them? Do you find the implementations and docstrings ok? |
|
Same issue. Currently the bot works only on PRs from branches of this repo. PRs from forks fail. All tests pass, so that is not a blocker. I would suggest editing the PR description to actually describe what this PR is about. |
I haven't had the time to look at it entirely - but I wrote a comment yesterday that somehow did not get posted: I was wondering about the behavior for loops. Currently a graph with edges |
src/operators.jl
Outdated
| end | ||
| end | ||
|
|
||
| foreach(sort!, edge_to_neighbors) |
There was a problem hiding this comment.
Maybe check if my reasoning is wrong - but I think sorting is not necessary here because edges already return the edges in lexicographical order.
There was a problem hiding this comment.
Hmm, the docstring of edges does not say we can count on the lexicographical order of iteration. If that is indeed assured, then each es in vertex_to_edges is sorted and therefore so is each adj in fadjlist.
So, what is best to do?
(1) Keep sort!, just in case? (no changes)
(2) Replace sort! with @assert all(issorted(adj) for adj in fadjlist)?
(3) Remove sort and count on edges providing lexicographic sorting?
|
So the literature is quite sparse with regards to line graphs and self-loops but I just checked the behavior of networkx and igraph on a single vertex with a self-loop. For directed graphs they line graph for both packages result in a single vertex with a self-loop again. For undirected graphs, networkx produces a single vertex without any loops. Igraph produces a single vertex with a self-loop. |
simonschoelly
left a comment
simonschoelly
left a comment
There was a problem hiding this comment.
I think the tests are mostly good - what I noticed though that you only run them for SimpleGraph{Int} and SimpleDiGraph{Int} - just to make sure they also work correctly for other eltypes I would add a few tests for example for SimpleGraph{UInt8}.
Done. |
Ok, I did the test for the smallest (di)graph (2 vertices) for Int8,...,Int128,Uint8,...,UInt128. |
If so, by the same 'logic', shouldn't there also be an edge
I just checked I don't know. I'll do as you suggest. Edit: @simonschoelly I could also add an optional argument |
@simonschoelly What is your decision regarding self-loops in a line-graph? Shall I add an optional argument, like Also, what is your decision regarding I'd like to finish this PR in a timely manner. |
Sorry for interrupting the discussion :) Maybe it should follow from the definition of line graphs, as it make easier to understand the output. For the undirected case, we have a single loopless vertex. Then the options would not be necessary (unless for some specific reason?). SageMath seems to follow this convention (https://doc.sagemath.org/html/en/reference/graphs/sage/graphs/line_graph.html#module-sage.graphs.line_graph) when mentioning «Loops are not adjacent to themselves». For example, consider the P_3 {{1, 2}, {2}, {2, 3}} with a self-loop in the middle vertex. The intersection graph of the edges is a triangle. |
|
Hi, @lampretl ! Apologies for letting the momentum here die out. I would like to try to help to get this merged. It seems almost ready. A couple of comments:
I will mark this as a draft for now to keep my review queue a bit more organized, but do not hesitate to convert it back if you have the energy and desire to finish it. Thank you for sticking around, especially given the slow pace at which we get to do reviews! |
|
@elvcastelo Hey, no worries, you are not interrupting at all :). But I don't understand your comment. How do you suggest self-loops in @Krastanov Give me a few days, I will refresh my memory about this PR and edit it as necessary. Hopefully, I'll get replies a bit faster so the code can be finished and merged into main in a timely manner. : ) |
|
Thanks, Leon! We are struggling to get enough volunteer reviewers, so please do not hesitate to "bump" with reminders to review -- these are actually appreciated. Apologies for having to ask you to do so. Relatedly, if you feel like it, do not hesitate to provide reviews to existing PRs -- those are greatly appreciated. |
|
To me it seems the main definition on wikipedia assumes that we are dealing with simple graphs, so no self loops are allowed anyway. We could just do the same here, and throw an error when the given graph has self loops. Similarly, the line graph L(G) has to be simple as well, otherwise the definition would have self loops all over, as any edge (i-j) has a common endpoint with itself. (The question whether a self loop in G should cause a self loop in L(G) boils down to whether we check adjacency of an edge with itself. Doing this selectively only for self loops seems very arbitrary to me.) changing the definition to That way, a loop (or any edge for that matter...) is never adjacent to itself, but is adjacent to any edge going in or out of the looped vertex. I think this should generalize nicely to the directed case as well. |
5 participants
I added the function
line_graph(one method for undirected graphs, one for directed graphs) and several unit tests that check the correctness of the output graph on simple cases (cycle, path, star graph, and a small graph with 2 or 3 vertices and a loop).I also edited
runtests.jlto useARGS, so that when runningonly those two tests are performed. This was necessary, because running the whole set of tests took 15min on my computer (6-core i7-8850H CPU), but doing only those two tests took <50sec, so it was easier to iterate.