Fix issymmetric #265
Fix issymmetric #265
Conversation
Codecov Report
@@ Coverage Diff @@
## master #265 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 114 114
Lines 6649 6656 +7
=======================================
+ Hits 6461 6468 +7
Misses 188 188 |
| gx = SimpleDiGraph(4) | ||
| add_edge!(gx, 1, 2) | ||
| add_edge!(gx, 2, 1) | ||
| add_edge!(gx, 1, 3) | ||
| add_edge!(gx, 3, 1) | ||
| @testset "Matrix operations: $g" for g in testdigraphs(gx) | ||
| @test @inferred(issymmetric(g)) | ||
| end | ||
|
|
There was a problem hiding this comment.
Can we rewrite this test in terms of GenericDiGraph, now that we have it?
There was a problem hiding this comment.
I didn't check but I don't think Generic(Di)Graph is used anywhere in the tests yet? The corresponding issue #133 is still open and doesn't have a PR associated with it?
There was a problem hiding this comment.
I agree that in time GenericGraph should be everywhere, but inserting it in only one spot seems a bit weird, kinda like removing the "not implemented" error the other day 😉
There was a problem hiding this comment.
It is indeed only uses somehwhere in traversals/dfs.jl. We don't have to insert it here, I just approve it for now, although in general I think we can suggest it in new PRs.
I haven't gotten around, but I guess then I will start creating some PRs that will switch to GenericGraph - I don't want to do the whole Graphs package in one PR though, that will be way too big and newer get reviewed.
There was a problem hiding this comment.
Maybe GenericGraph could wait for GraphsBase before spreading through our tests?
JuliaGraphs/GraphsBase.jl#6
|
Mostly looks good to me, perhaps that one issue about the tests but otherwise I am all for merging. |
Solve #248