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

LatticePoset: add atoms, coatoms, doubly irreducibles etc. #19190

Closed
jm58660 mannequin opened this issue Sep 11, 2015 · 20 comments
Closed

LatticePoset: add atoms, coatoms, doubly irreducibles etc. #19190

jm58660 mannequin opened this issue Sep 11, 2015 · 20 comments

Comments

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Sep 11, 2015

Add atoms(), coatoms(), and doubly_irreducibles() to finite lattices.

Component: combinatorics

Keywords: latticeposet

Author: Jori Mäntysalo

Branch: aa58591

Reviewer: Travis Scrimshaw

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

@jm58660 jm58660 mannequin added c: combinatorics labels Sep 11, 2015
@jm58660

This comment has been minimized.

@jm58660

This comment has been minimized.

@jm58660 jm58660 mannequin changed the title LatticePoset: add atoms, coatoms, doubly irreducibles LatticePoset: add atoms, coatoms, doubly irreducibles etc. Sep 19, 2015
@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Apr 13, 2016

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Apr 13, 2016

comment:4

This patch will add three functions. Also this will make LatticePoset() to return the empty lattice; compare to Poset().


New commits:

429b494Add atoms() etc.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Apr 13, 2016

Changed keywords from poset to latticeposet

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Apr 13, 2016

Commit: 429b494

@jm58660

This comment has been minimized.

@jm58660 jm58660 mannequin added the s: needs review label Apr 13, 2016
@jm58660 jm58660 mannequin added this to the sage-7.2 milestone Apr 13, 2016
@jm58660 jm58660 mannequin removed the wishlist item label Apr 13, 2016
@tscrim
Copy link
Collaborator

tscrim commented Apr 13, 2016

comment:5

On my to-review list.

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2016

Changed commit from 429b494 to aa58591

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2016

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2016

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2016

comment:6

I did some (additional) touchups to the finite lattice category and minor tweaks. While I still prefer ``self`` over this lattice, the lattice is not correct as there is not a unique lattice. If you agree with my changes, then go ahead and set a positive review.


New commits:

376adfbSome cleanup in the category docstrings.
aa58591Some touchups of lattices.py.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Apr 20, 2016

comment:7

In html documentation "See also: FinitePosets, LatticePosets, LatticePoset" the "LatticePoset" is a broken link. Where is it supposed to point?

About "the": I don't understand. For example docstring for cardinality() is "Return the number of elements in the poset." I have think that "the" means about same as "this" in that sentence.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Apr 20, 2016

comment:8

Btw, I run doctests and they were successfull. So patchbots again give false errors.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Apr 23, 2016

comment:9

Anyways, Sage with this patch is better than without, hence I mark this as positive_review.

Broken links are more general problem. Nathann give one suggestion at #20095, but that should be talked separately.

@vbraun
Copy link
Member

vbraun commented Apr 23, 2016

Changed branch from public/posets/add_methods-19190 to aa58591

@tscrim
Copy link
Collaborator

tscrim commented Apr 23, 2016

comment:11

"the" implies uniqueness, but there is not a unique poset. "this" within the context gives uniqueness. In fact, I would change all of those "the" to "this".

@tscrim
Copy link
Collaborator

tscrim commented Apr 23, 2016

Changed commit from aa58591 to none

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Apr 24, 2016

comment:12

Replying to @tscrim:

"the" implies uniqueness, but there is not a unique poset. "this" within the context gives uniqueness. In fact, I would change all of those "the" to "this".

Trying to understand... There is no direct translation for a/an/the in Finnish.

I have learnt that if I say "Travis, open the window", it means that I have one specific window in my mind and I suppose that you also know what window I mean. "Open a window" means that there are several to choose one, and I don't care which one you open.

In "Return the number of elements in the poset." I suppose that it is clear what poset we are referring to.

@tscrim
Copy link
Collaborator

tscrim commented Apr 24, 2016

comment:13

Replying to @jm58660:

Replying to @tscrim:

"the" implies uniqueness, but there is not a unique poset. "this" within the context gives uniqueness. In fact, I would change all of those "the" to "this".

Trying to understand... There is no direct translation for a/an/the in Finnish.

I have absolutely zero understanding of Finnish (I don't think I've ever really encountered any before).

I have learnt that if I say "Travis, open the window", it means that I have one specific window in my mind and I suppose that you also know what window I mean. "Open a window" means that there are several to choose one, and I don't care which one you open.

Yes, provided there is only one window we are discussing.

In "Return the number of elements in the poset." I suppose that it is clear what poset we are referring to.

In a way, yes, but it is more precise to say "this poset" as there might be one more poset around (especially when there is another input of a poset).

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