Implement IsBetweenCoverAndLattice#920
Implement IsBetweenCoverAndLattice#920ThatOtherAndrew wants to merge 7 commits intodigraphs:mainfrom
Conversation
|
gaplint should be satisfied now :> |
There was a problem hiding this comment.
Pull request overview
Adds a new predicate to determine whether a digraph lies between the cover relation of some lattice and a lattice digraph (without constructing the reflexive transitive closure), motivated by #714.
Changes:
- Introduces
IsBetweenCoverAndLatticeas a declared property and implements its method ingap/prop.gi. - Adds
DIGRAPHS_MeetJoinTableBetweenCover, a meet/join table builder that avoids relying on transitive edges being present. - Adds standard tests covering basic positive/negative cases for
IsBetweenCoverAndLattice.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
gap/prop.gi |
Implements IsBetweenCoverAndLattice and adds the new meet/join table helper used by it. |
gap/prop.gd |
Declares the IsBetweenCoverAndLattice property and installs a trivial-true implication for lattice digraphs. |
tst/standard/prop.tst |
Adds regression tests for IsBetweenCoverAndLattice across several small digraph examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (95.23%) 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 #920 +/- ##
==========================================
+ Coverage 97.35% 97.37% +0.02%
==========================================
Files 50 50
Lines 21045 21256 +211
Branches 639 639
==========================================
+ Hits 20489 20699 +210
- Misses 491 492 +1
Partials 65 65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for the commit! I have suggested some changes. One larger change I would like is for you to modify DIGRAPHS_MeetJoinTable to have the behavior of DIGRAPHS_MeetJoinTableBetweenCover depending on a switch.
One other thing: The function is currently missing documentation. To add this you should modify the doc/prop.xml file and add an entry to this file for the IsBetweenCoverAndLattice function, see the doc for IsMeetSemilatticeDigraph function doc for an example:
Lines 1295 to 1349 in 313228b
You then want to include the function doc in the manual, the doc/z-chap5.xml file in the orders section after IsMeetSemilatticeDigraph is probably the right place for this:
Lines 41 to 51 in 313228b
Otherwise looks good in the first instance!
| # the below conditions are the only part that | ||
| # differs from MeetJoinTable above | ||
| if join and tab[q, z] <> z then | ||
| return fail; | ||
| elif not join and tab[z, q] <> z then | ||
| return fail; |
There was a problem hiding this comment.
Given that this is the only difference, would it be feasible to modify DIGRAPHS_MeetJoinTable to have both functionalities e.g. by adding an extra boolean parameter check_edges, which makes the DIGRAPHS_MeetJoinTable behave the same as before if set to true and otherwise make it behave like DIGRAPHS_MeetJoinTableBetweenCover?
There was a problem hiding this comment.
On it! Would doing a text search for "DIGRAPHS_MeetJoinTable" be sufficient to ensure that all places affected by the function signature breaking change are covered?
There was a problem hiding this comment.
Yes I think so! It would also be nice if you could add a comment describing what each of the input parameters D, P, U, join and the new one you add is expected to be, and how using them affects the output. The DIGRAPHS_ prefix functions are for internal use only, so they don't get an official documentation entry, but it might be useful for the next person who touches the code.
reiniscirpons
left a comment
There was a problem hiding this comment.
A few more things I noticed. We should discuss next meeting, but overall looks good.
| neighbours := OutNeighbours(hasse); | ||
| table := DIGRAPHS_MeetJoinTableBetweenCover( | ||
| DigraphNrVertices(copy), | ||
| order, | ||
| neighbours, | ||
| true); | ||
| return table <> fail; |
There was a problem hiding this comment.
Given the similarity of this line to the previous one, would it might make more sense to have two functions IsBetweenCoverAndMeetSemilattice and IsBetweenCoverAndJoinSemilattice which to this check with the meet and join table respectively and then make IsBetweenCoverAndLattice to simply return true when both IsBetweenCoverAndMeetSemilattice and IsBetweenCoverAndJoinSemilattice are true, like we do for IsLatticeDigraph.
| if table = fail then | ||
| return false; | ||
| fi; | ||
|
|
There was a problem hiding this comment.
| if IsImmutableDigraph(D) then | |
| SetDigraphMeetTable(D, tab); | |
| fi; |
We can save the result as the meet table here, since we already did all that work to compute it. The same should be done for the join table below.
Co-authored-by: Reinis Cirpons <43414125+reiniscirpons@users.noreply.github.com>
Co-authored-by: Reinis Cirpons <43414125+reiniscirpons@users.noreply.github.com>
Prompted by conversation in #714, I believe this is a functional implementation of the
IsBetweenCoverAndLatticefunction. My implementation is based on the three-step outline given by @Joseph-Edwards and also implementsDIGRAPHS_MeetJoinTableBetweenCover(as a slightly modified/generalised version ofDIGRAPHS_MeetJoinTable) in order to make this possible. I've thrown in some tests as well (due to my limited understanding of the maths I'm unsure how comprehensive they are though!) and they do appear to pass when building and running locally. (hopefully CI/CD can validate if they still work!)