Opened 8 years ago

Closed 8 years ago

#18515 closed enhancement (fixed)

LatticePoset: add sublattice()

Reported by: jmantysalo Owned by:
Priority: major Milestone: sage-6.8
Component: combinatorics Keywords:
Cc: chapoton, ncohen Merged in:
Authors: Jori Mäntysalo, Nathann Cohen Reviewers: Nathann Cohen, Jori Mäntysalo
Report Upstream: N/A Work issues:
Branch: a86919a (Commits, GitHub, GitLab) Commit: a86919aedb982998f23bd2c6eaf523caf1562c27
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description

Add a function to get smallest sublattice of the lattice containing given elements.

Change History (12)

comment:1 Changed 8 years ago by jmantysalo

Branch: u/jmantysalo/latticeposet__add_sublattice__

comment:2 Changed 8 years ago by jmantysalo

Authors: Jori Mäntysalo
Cc: chapoton ncohen added
Commit: 54d912c9e08792b278a6e86b37a572f87f27746f
Status: newneeds_review

Slooow. Better than nothing. Needs review.


New commits:

6b9a03bAdded a function.
54d912cRemoved spaces.

comment:3 Changed 8 years ago by git

Commit: 54d912c9e08792b278a6e86b37a572f87f27746f34a588d1878d613b8e36736a625be1e49d67be77

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

34a588dArghs, should return a lattice, not a poset.

comment:4 Changed 8 years ago by deinst

Branch: u/jmantysalo/latticeposet__add_sublattice__u/deinst/latticeposet__add_sublattice__

comment:5 Changed 8 years ago by deinst

Commit: 34a588d1878d613b8e36736a625be1e49d67be7747f2ce7a7799982ea29157c932bf91b31b78a3cc

I think there is a typo. I've changed the code to what I think is correct. The previous code did the following:

sage: L = Posets.BooleanLattice(3)
sage: L.sublattice([3,5,6,7])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-14-837853635c8a> in <module>()
----> 1 L.sublattice([Integer(3),Integer(5),Integer(6),Integer(7)])

/Users/davideinstein/projects/sage/local/lib/python2.7/site-packages/sage/combinat/posets/lattices.pyc in sublattice(self, elms)
    881                     elms.append(m)
    882                     n=True
--> 883         return LatticePoset(self.subposet(elms))
    884 
    885 ############################################################################

/Users/davideinstein/projects/sage/local/lib/python2.7/site-packages/sage/combinat/posets/lattices.pyc in LatticePoset(data, *args, **options)
    406     P = Poset(data, *args, **options)
    407     if not P.is_lattice():
--> 408         raise ValueError("Not a lattice.")
    409 
    410     return FiniteLatticePoset(P, category = FiniteLatticePosets(), facade = P._is_facade)

ValueError: Not a lattice.

Also, could you put whitespace around = as suggested by http://doc.sagemath.org/html/en/developer/coding_basics.html#python-code-style


New commits:

47f2ce7Fixed probable typo

comment:6 Changed 8 years ago by deinst

Otherwise it looks good to me.

comment:7 Changed 8 years ago by jmantysalo

Thanks! And yes, there was a stupid typo. I'll try to remember spaces from now on. (A promise I have made before...)

So, who's going to review this?

comment:8 in reply to:  7 Changed 8 years ago by ncohen

Yo !

I added a commit at public/18515 which improves the algorithm a bit. In general, try to avoid using 'Subsets' (or any combinat stuff) if you can avoid it: they implement their code to deal with infinitely more abstraction than you usually need (categories, parents, elements, ...).

sage: type(Subsets([1,2,3],2).an_element())
<class 'sage.sets.set.Set_object_enumerated_with_category'>

When you only need to enumerate pairs of points, the better is to use itertools:

sage: from itertools import combinations
sage: list(combinations([1,2,3],2))
[(1, 2), (1, 3), (2, 3)]

Simple, and more efficient.

So, who's going to review this?

We can review each other's commits, it is not forbidden. For as long as everything is doubled checked, no problem.

Nathann

comment:9 Changed 8 years ago by jmantysalo

Seems good, but of course doesn't make that much difference than totally other algorithm mentioned in the note. I'll check the code of Nathann and then put this to positive review. (Assuming that deinst doesn't do it before that.)

comment:10 Changed 8 years ago by jmantysalo

Authors: Jori MäntysaloNathann Cohen
Branch: u/deinst/latticeposet__add_sublattice__public/18515
Commit: 47f2ce7a7799982ea29157c932bf91b31b78a3cca86919aedb982998f23bd2c6eaf523caf1562c27
Reviewers: Jori Mäntysalo
Status: needs_reviewpositive_review

Works.


New commits:

a86919atrac #18515: A less wasteful algorithm

comment:11 Changed 8 years ago by ncohen

Authors: Nathann CohenJori Mäntysalo, Nathann Cohen
Reviewers: Jori MäntysaloNathann Cohen, Jori Mäntysalo

;-)

comment:12 Changed 8 years ago by vbraun

Branch: public/18515a86919aedb982998f23bd2c6eaf523caf1562c27
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.