Repository navigation
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (91.66%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #922 +/- ##
==========================================
- Coverage 97.48% 97.43% -0.05%
==========================================
Files 53 53
Lines 21363 21519 +156
Branches 639 639
==========================================
+ Hits 20825 20968 +143
- Misses 473 486 +13
Partials 65 65 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| # reconstruct graph from the planar bipartite graph | ||
| # for the inputted H we have U = [1...n] and V is the rest(all junctions) | ||
| BindGlobal("DIGRAPHS_HalfSquare", |
There was a problem hiding this comment.
I think this should be an attribute (or maybe an operation), for a bipartite graph, that has independent documentation and tests. It should probably take which set of vertices to use (instead of [1 .. n]), and check if that set of vertices is one of the two sets used in the definition of the bipartite graph.
| m := DigraphNrAdjacenciesWithoutLoops(G); | ||
|
|
||
| if n < 5 or m < 9 then | ||
| return true; |
There was a problem hiding this comment.
be good to combine these ifs into a single statement using elif, and possibly providing a reference for why these conditions imply the answers they do.
| Add(canon, v); | ||
| fi; | ||
| Sort(canon); | ||
| key := String(canon); |
There was a problem hiding this comment.
I don't think this will work in general, how is the key "123" interpreted? It can be generated by [1, 23], and [123], and there's no way to tell the difference. I think you maybe want to use a HashMap here instead of a record.
| for i in [1 .. Length(nbrs)] do | ||
| mapping[nbrs[i]] := i; | ||
| od; | ||
|
|
There was a problem hiding this comment.
Local functions that depend on the state of "global" variables, like subEdges here, smell bad, and should probably be avoided. Can you please refactor this so that EdgesCoveredBy is a global function and that it's arguments are explicitly given?
| subEdges := []; | ||
| for i in DigraphVertices(sub) do | ||
| for e in OutNeighboursOfVertex(sub, i) do | ||
| if e > i then |
There was a problem hiding this comment.
I'm struggling to understand why e > i is being checked here? Be good to have some more comments to understand what's being done here.
There was a problem hiding this comment.
Intent is so that we don't double count edges([1,2] and [2,1]) ordering doesn't matter but edges are undirected(as a cant neighbor b without b also neighboring a)
There was a problem hiding this comment.
Gotcha, I'd still like to understand what algorithm your implementing but we can discuss it tomorrow, looks like there's great stuff in here!
ec289fc to
7a73ede
Compare
james-d-mitchell
left a comment
james-d-mitchell
left a comment
There was a problem hiding this comment.
@William-Kenyon thanks for working on this PR, it's really helpful! There are still a couple of unresolved comments from my earlier review, can you please take a look at those too?
The other thing that's missing, which is the real blocker for merging this is that I don't know what algorithm @anujthak originally implemented. I had intended to ask @anujthak at the final VIP meeting last semester, but the opportunity did not arise. @William-Kenyon if you know what algorithm is being implemented, can you add a description as a comment somewhere in the code? I mean a high-level explanation of what is being done (not how).
This branch has not been deployed
3 participants
Adding an algorithm in response to #546 that recognizes whether or not an inputted graph is a map graph.