Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LatticePosets: Add congruence-related functions / part 2 #21861

Closed
jm58660 mannequin opened this issue Nov 11, 2016 · 22 comments
Closed

LatticePosets: Add congruence-related functions / part 2 #21861

jm58660 mannequin opened this issue Nov 11, 2016 · 22 comments

Comments

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Nov 11, 2016

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.

Depends on #22225

CC: @mantepse @tscrim @fchapoton

Component: combinatorics

Author: Jori Mäntysalo

Branch/Commit: 5fc745e

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/21861

@jm58660 jm58660 mannequin added this to the sage-7.5 milestone Nov 11, 2016
@jm58660 jm58660 mannequin added c: combinatorics labels Nov 11, 2016
@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jan 30, 2017

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jan 30, 2017

comment:3

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.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jan 30, 2017

Commit: d96afed

@jm58660

This comment has been minimized.

@jm58660 jm58660 mannequin modified the milestones: sage-7.5, sage-7.6 Jan 30, 2017
@jm58660 jm58660 mannequin added the s: needs review label Jan 30, 2017
@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Feb 4, 2017

comment:4

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

@jm58660

This comment has been minimized.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Feb 12, 2017

comment:5

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 12, 2017

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

dd5be6cMerge branch 'develop' into t/21861/latticeposets__add_congruence_related_functions___part_2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 12, 2017

Changed commit from d96afed to dd5be6c

@tscrim
Copy link
Collaborator

tscrim commented Feb 12, 2017

comment:7

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 12, 2017

Changed commit from dd5be6c to 699f24d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 12, 2017

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

699f24dDoctest a bug correction.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Feb 12, 2017

comment:9

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.

@tscrim
Copy link
Collaborator

tscrim commented Feb 13, 2017

comment:10

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.)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 13, 2017

Changed commit from 699f24d to 5fc745e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 13, 2017

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

5fc745eRemoved todo-notes.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Feb 13, 2017

Dependencies: #22225

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Feb 13, 2017

comment:12

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.)

@tscrim
Copy link
Collaborator

tscrim commented Feb 14, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Feb 14, 2017

comment:13

Replying to @jm58660:

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.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Feb 14, 2017

comment:14

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

@vbraun
Copy link
Member

vbraun commented Feb 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants