Opened 6 years ago

Closed 5 years ago

#21861 closed enhancement (fixed)

LatticePosets: Add congruence-related functions / part 2

Reported by: jmantysalo Owned by:
Priority: major Milestone: sage-7.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:

Status badges

Description (last modified by jmantysalo)

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.

Related: #20058, #22225, #22238, #22229.

Change History (15)

comment:1 Changed 6 years ago by jmantysalo

Last edited 6 years ago by jmantysalo (previous) (diff)

comment:2 Changed 6 years ago by jmantysalo

  • Branch set to u/jmantysalo/latticeposets__add_congruence_related_functions___part_2

comment:3 Changed 6 years ago by jmantysalo

  • Cc tscrim chapoton added
  • Commit set to d96afeddf02ee160ad730922e2e6bc4335b315e3
  • Description modified (diff)
  • Milestone changed from sage-7.5 to sage-7.6
  • Status changed from new to needs_review

Whole set: Needs comments about the code implementation, the code PEP-compliance, the documentation and the examples used.


New commits:

d96afedAdd three congruence-related frontend-functions, two to backend.

comment:4 Changed 6 years ago by jmantysalo

If wanted, I can of course split this ticket to several separate tickets.

comment:5 Changed 6 years ago by jmantysalo

  • Description modified (diff)

One doctest fails, but that is already taken care by #22225.

comment:6 Changed 6 years ago by git

  • Commit changed from d96afeddf02ee160ad730922e2e6bc4335b315e3 to dd5be6c09034959968ea92ca6e32164472d30895

Branch pushed to git repo; I updated commit sha1. New commits:

dd5be6cMerge branch 'develop' into t/21861/latticeposets__add_congruence_related_functions___part_2

comment:7 follow-up: Changed 6 years ago by tscrim

Can you add a test showing the bug is fixed? Also, what about the TODO comment?

comment:8 Changed 6 years ago by git

  • Commit changed from dd5be6c09034959968ea92ca6e32164472d30895 to 699f24d90815d2156dbe9ff5dd1a1f17edb4e05d

Branch pushed to git repo; I updated commit sha1. New commits:

699f24dDoctest a bug correction.

comment:9 in reply to: ↑ 7 Changed 6 years ago by jmantysalo

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 follow-up: Changed 6 years ago by 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.

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 git

  • Commit changed from 699f24d90815d2156dbe9ff5dd1a1f17edb4e05d to 5fc745e0281756b99acb88dc63eabf6969b5a943

Branch pushed to git repo; I updated commit sha1. New commits:

5fc745eRemoved todo-notes.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 6 years ago by jmantysalo

  • 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 tscrim

  • 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 jmantysalo

Thanks Travis! Not only for this one, but for several tickets.

comment:15 Changed 5 years ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.