Opened 6 years ago
Closed 5 years ago
#21861 closed enhancement (fixed)
LatticePosets: Add congruencerelated functions / part 2
Reported by:  jmantysalo  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  combinatorics  Keywords:  
Cc:  mantepse, tscrim, chapoton  Merged in:  
Authors:  Jori Mäntysalo  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  5fc745e (Commits, GitHub, GitLab)  Commit:  5fc745e0281756b99acb88dc63eabf6969b5a943 
Dependencies:  #22225  Stopgaps: 
Description (last modified by )
This patch adds functions to check if the lattice is regular, uniform or isoform.
Second, "backend" functions will make it easier to construct the lattice of congruences.
Third, there was a slight error in congruence()
of Hasse diagram with start
parameter. This patch corrects that.
Change History (15)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
 Branch set to u/jmantysalo/latticeposets__add_congruence_related_functions___part_2
comment:3 Changed 6 years ago by
 Cc tscrim chapoton added
 Commit set to d96afeddf02ee160ad730922e2e6bc4335b315e3
 Description modified (diff)
 Milestone changed from sage7.5 to sage7.6
 Status changed from new to needs_review
comment:4 Changed 6 years ago by
If wanted, I can of course split this ticket to several separate tickets.
comment:5 Changed 6 years ago by
 Description modified (diff)
One doctest fails, but that is already taken care by #22225.
comment:6 Changed 6 years ago by
 Commit changed from d96afeddf02ee160ad730922e2e6bc4335b315e3 to dd5be6c09034959968ea92ca6e32164472d30895
Branch pushed to git repo; I updated commit sha1. New commits:
dd5be6c  Merge branch 'develop' into t/21861/latticeposets__add_congruence_related_functions___part_2

comment:7 followup: ↓ 9 Changed 6 years ago by
Can you add a test showing the bug is fixed? Also, what about the TODO
comment?
comment:8 Changed 6 years ago by
 Commit changed from dd5be6c09034959968ea92ca6e32164472d30895 to 699f24d90815d2156dbe9ff5dd1a1f17edb4e05d
Branch pushed to git repo; I updated commit sha1. New commits:
699f24d  Doctest a bug correction.

comment:9 in reply to: ↑ 7 Changed 6 years ago by
Replying to tscrim:
Can you add a test showing the bug is fixed?
Good point. Added.
Also, what about the
TODO
comment?
Well, I haven't thinked yet :=)
. The code works, but it can be that it does unnecessary computing.
comment:10 followup: ↓ 12 Changed 6 years ago by
This hits the same error on the number of functions as the other ticket (sorry, I don't remember the number offhand), so that is a (soft) dependency of this ticket.
If you haven't thought about it yet and don't plan on doing so soon, then could you change the TODO
comment to seem less like it is targeted to yourself? (Sorry, I don't really understand the comment, so I can't quite help here.)
comment:11 Changed 6 years ago by
 Commit changed from 699f24d90815d2156dbe9ff5dd1a1f17edb4e05d to 5fc745e0281756b99acb88dc63eabf6969b5a943
Branch pushed to git repo; I updated commit sha1. New commits:
5fc745e  Removed todonotes.

comment:12 in reply to: ↑ 10 ; followup: ↓ 13 Changed 6 years ago by
 Dependencies set to #22225
Replying to tscrim:
This hits the same error on the number of functions as the other ticket (sorry, I don't remember the number offhand), so that is a (soft) dependency of this ticket.
Added.
If you haven't thought about it yet and don't plan on doing so soon, then could you change the
TODO
comment to seem less like it is targeted to yourself?
As you wish, removed. I guess everyone looking at the code can see that there might be a place for optimization.
The question can be phrased like this: "Suppose that the congruence generated by pair (a,b) is regular, and so is the congruence generated by (c,d). Is the congruence generated by both together then also regular?" (And same for uniform and isoform congruences.)
comment:13 in reply to: ↑ 12 Changed 5 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Replying to jmantysalo:
Replying to tscrim:
If you haven't thought about it yet and don't plan on doing so soon, then could you change the
TODO
comment to seem less like it is targeted to yourself?As you wish, removed. I guess everyone looking at the code can see that there might be a place for optimization.
Thanks. Positive review.
The question can be phrased like this: "Suppose that the congruence generated by pair (a,b) is regular, and so is the congruence generated by (c,d). Is the congruence generated by both together then also regular?" (And same for uniform and isoform congruences.)
Alas, I have no idea, sorry.
comment:14 Changed 5 years ago by
Thanks Travis! Not only for this one, but for several tickets.
comment:15 Changed 5 years ago by
 Branch changed from u/jmantysalo/latticeposets__add_congruence_related_functions___part_2 to 5fc745e0281756b99acb88dc63eabf6969b5a943
 Resolution set to fixed
 Status changed from positive_review to closed
Whole set: Needs comments about the code implementation, the code PEPcompliance, the documentation and the examples used.
New commits:
Add three congruencerelated frontendfunctions, two to backend.