Opened 4 years ago
Closed 4 years ago
#23111 closed enhancement (fixed)
Remove is_distributive_lattice() from hasse_diagram.py
Reported by:  jmantysalo  Owned by:  

Priority:  minor  Milestone:  sage8.1 
Component:  combinatorics  Keywords:  
Cc:  tscrim  Merged in:  
Authors:  Jori Mäntysalo  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  bca22b3 (Commits, GitHub, GitLab)  Commit:  bca22b361b482130c53e0516fbcb55567a78f2f8 
Dependencies:  Stopgaps: 
Description
This patch removes a useless function. (Testing for distributivity can be done much faster.)
Hopefully someday I (or someone other) will do #17173.
Change History (20)
comment:1 Changed 4 years ago by
 Branch set to u/jmantysalo/removedist
comment:2 Changed 4 years ago by
 Cc tscrim added
 Commit set to f2326287a4e567392c15a2a9ee631ddda931153e
 Status changed from new to needs_review
comment:3 Changed 4 years ago by
 Status changed from needs_review to needs_info
Until there is an alternative, I propose we leave this in. Better to have some implementation than none at all. If you do really want to remove this function, then please justify it a little more and deprecate it.
comment:4 followup: ↓ 5 Changed 4 years ago by
We still have .is_distributive
for lattices. But one first need to check if the poset is a lattice. I have not compared the speeds.
comment:5 in reply to: ↑ 4 Changed 4 years ago by
Replying to chapoton:
We still have
.is_distributive
for lattices. But one first need to check if the poset is a lattice. I have not compared the speeds.
It is clear that the function I suggest to remove is slowest.
First, it checks if a poset is a lattice by computing meet and joinmatrices; only other is needed, if the poset is bounded.
Second, this is the most trivial implementation. It is much faster to just check Ji(L)=Mi(L)=h(L)
and L
is graded.
comment:6 Changed 4 years ago by
Btw, a dismantlable and bounded poset is actually a dismantlable lattice. Do we want functions is_*_lattice()
to posets.py
in general? I do not know if there is, say, faster is_relatively_complemented_lattice()
than just trying LatticePoset(P).is_relatively_complemented()
.
comment:7 followup: ↓ 9 Changed 4 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_info to needs_work
On one had, you have to check it is a lattice by converting to a lattice, which I believe is somewhat expensive. However, you may not always have lattices in some set of posets, so having such a method at the higher level is good. A split the difference approach would be to have methods with more general checks for posets at the poset level, but this would make things inconsistent in a way. However, this is not a good reason as the HasseDiagram
is considered (IMO) an implementation detail, and essentially hidden from the user. So, it makes sense to have methods that do not need a general checks for latticeness.
I guess in this case I have just convinced myself that removing this method is the proper thing to do. However, because it is part of the public API of HasseDiagram
, we still need to deprecate it.
comment:8 Changed 4 years ago by
 Commit changed from f2326287a4e567392c15a2a9ee631ddda931153e to 40aa1fe5fb8090abcc5f60b071ccb5c94fc081de
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
40aa1fe  Faster is_distributive for hasse_diagram.py.

comment:9 in reply to: ↑ 7 Changed 4 years ago by
Replying to tscrim:
I guess in this case I have just convinced myself that removing this method is the proper thing to do. However, because it is part of the public API of
HasseDiagram
, we still need to deprecate it.
Then it is easier just to make this faster.
Uncomplied code committed, not ready for review.
comment:10 Changed 4 years ago by
 Commit changed from 40aa1fe5fb8090abcc5f60b071ccb5c94fc081de to b6ba872027e3f39ad799652625f185d2a1cc6837
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b6ba872  Add deprecation.

comment:11 Changed 4 years ago by
Getting back to this one... Is this the right way to start deprecating?
comment:12 followup: ↓ 13 Changed 4 years ago by
 Milestone changed from sage8.0 to sage8.1
Almost. You should still have a doctest showing the deprecation.
comment:13 in reply to: ↑ 12 Changed 4 years ago by
Replying to tscrim:
Almost. You should still have a doctest showing the deprecation.
But the devel manual says "It will display the message of your choice (and interact properly with the doctest framework)", i.e. if I just left the EXAMPLES
block as it is, it will pass the doctest. Hence I don't know what to do.
comment:14 Changed 4 years ago by
No, the unchanged doctest will not pass. Just keep one simple doctest, looking like that
sage: p = MixedIntegerLinearProgram(solver='GLPK') sage: p.linear_function({1:3, 4:5}) doctest:...: DeprecationWarning:...linear_function...deprecated... 3*x_1 + 5*x_4
comment:15 Changed 4 years ago by
 Commit changed from b6ba872027e3f39ad799652625f185d2a1cc6837 to 9e95fc053d6dfa9bdb3dd77fc0dca818cb5cc441
Branch pushed to git repo; I updated commit sha1. New commits:
9e95fc0  Add test for deprecation.

comment:16 Changed 4 years ago by
Strange... I must have tested this without compiling first...
Anyways, now there is a test.
comment:17 followup: ↓ 19 Changed 4 years ago by
Triple quote is overindented. Once fixed, you can set a positive review on my behalf.
comment:18 Changed 4 years ago by
 Commit changed from 9e95fc053d6dfa9bdb3dd77fc0dca818cb5cc441 to bca22b361b482130c53e0516fbcb55567a78f2f8
Branch pushed to git repo; I updated commit sha1. New commits:
bca22b3  Indentation of doctest end marker.

comment:19 in reply to: ↑ 17 Changed 4 years ago by
 Status changed from needs_work to positive_review
Replying to tscrim:
Triple quote is overindented. Once fixed, you can set a positive review on my behalf.
Done this.
comment:20 Changed 4 years ago by
 Branch changed from u/jmantysalo/removedist to bca22b361b482130c53e0516fbcb55567a78f2f8
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Remove a function.