Opened 4 years ago

Closed 4 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) Commit: a86919aedb982998f23bd2c6eaf523caf1562c27
Dependencies: Stopgaps:

Description

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

Change History (12)

comment:1 Changed 4 years ago by jmantysalo

  • Branch set to u/jmantysalo/latticeposet__add_sublattice__

comment:2 Changed 4 years ago by jmantysalo

  • Authors set to Jori Mäntysalo
  • Cc chapoton ncohen added
  • Commit set to 54d912c9e08792b278a6e86b37a572f87f27746f
  • Status changed from new to needs_review

Slooow. Better than nothing. Needs review.


New commits:

6b9a03bAdded a function.
54d912cRemoved spaces.

comment:3 Changed 4 years ago by git

  • Commit changed from 54d912c9e08792b278a6e86b37a572f87f27746f to 34a588d1878d613b8e36736a625be1e49d67be77

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

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

comment:4 Changed 4 years ago by deinst

  • Branch changed from u/jmantysalo/latticeposet__add_sublattice__ to u/deinst/latticeposet__add_sublattice__

comment:5 Changed 4 years ago by deinst

  • 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)
<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 4 years ago by deinst

Otherwise it looks good to me.

comment:7 follow-up: Changed 4 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 4 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 4 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 4 years ago by jmantysalo

  • Authors changed from Jori Mäntysalo to Nathann Cohen
  • 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

Works.


New commits:

a86919atrac #18515: A less wasteful algorithm

comment:11 Changed 4 years ago by ncohen

  • Authors changed from Nathann Cohen to Jori Mäntysalo, Nathann Cohen
  • Reviewers changed from Jori Mäntysalo to Nathann Cohen, Jori Mäntysalo

;-)

comment:12 Changed 4 years ago by vbraun

  • Branch changed from public/18515 to a86919aedb982998f23bd2c6eaf523caf1562c27
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.