Opened 6 years ago
Closed 6 years ago
#18515 closed enhancement (fixed)
LatticePoset: add sublattice()
Reported by:  jmantysalo  Owned by:  

Priority:  major  Milestone:  sage6.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: 
Description
Add a function to get smallest sublattice of the lattice containing given elements.
Change History (12)
comment:1 Changed 6 years ago by
 Branch set to u/jmantysalo/latticeposet__add_sublattice__
comment:2 Changed 6 years ago by
 Cc chapoton ncohen added
 Commit set to 54d912c9e08792b278a6e86b37a572f87f27746f
 Status changed from new to needs_review
comment:3 Changed 6 years ago by
 Commit changed from 54d912c9e08792b278a6e86b37a572f87f27746f to 34a588d1878d613b8e36736a625be1e49d67be77
Branch pushed to git repo; I updated commit sha1. New commits:
34a588d  Arghs, should return a lattice, not a poset.

comment:4 Changed 6 years ago by
 Branch changed from u/jmantysalo/latticeposet__add_sublattice__ to u/deinst/latticeposet__add_sublattice__
comment:5 Changed 6 years ago by
 Commit changed from 34a588d1878d613b8e36736a625be1e49d67be77 to 47f2ce7a7799982ea29157c932bf91b31b78a3cc
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) <ipythoninput14837853635c8a> in <module>() > 1 L.sublattice([Integer(3),Integer(5),Integer(6),Integer(7)]) /Users/davideinstein/projects/sage/local/lib/python2.7/sitepackages/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/sitepackages/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#pythoncodestyle
New commits:
47f2ce7  Fixed probable typo

comment:6 Changed 6 years ago by
Otherwise it looks good to me.
comment:7 followup: ↓ 8 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
 Branch changed from u/deinst/latticeposet__add_sublattice__ to public/18515
 Commit changed from 47f2ce7a7799982ea29157c932bf91b31b78a3cc to a86919aedb982998f23bd2c6eaf523caf1562c27
 Reviewers set to Jori Mäntysalo
 Status changed from needs_review to positive_review
comment:11 Changed 6 years ago by
 Reviewers changed from Jori Mäntysalo to Nathann Cohen, Jori Mäntysalo
;)
comment:12 Changed 6 years ago by
 Branch changed from public/18515 to a86919aedb982998f23bd2c6eaf523caf1562c27
 Resolution set to fixed
 Status changed from positive_review to closed
Slooow. Better than nothing. Needs review.
New commits:
Added a function.
Removed spaces.