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: sage-8.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:

Status badges

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 jmantysalo

  • Branch set to u/jmantysalo/remove-dist

comment:2 Changed 4 years ago by jmantysalo

  • Cc tscrim added
  • Commit set to f2326287a4e567392c15a2a9ee631ddda931153e
  • Status changed from new to needs_review

New commits:

f232628Remove a function.

comment:3 Changed 4 years ago by tscrim

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

comment:5 in reply to: ↑ 4 Changed 4 years ago by jmantysalo

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 join-matrices; 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 jmantysalo

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

  • 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 lattice-ness.

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 git

  • Commit changed from f2326287a4e567392c15a2a9ee631ddda931153e to 40aa1fe5fb8090abcc5f60b071ccb5c94fc081de

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

40aa1feFaster is_distributive for hasse_diagram.py.

comment:9 in reply to: ↑ 7 Changed 4 years ago by jmantysalo

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 git

  • Commit changed from 40aa1fe5fb8090abcc5f60b071ccb5c94fc081de to b6ba872027e3f39ad799652625f185d2a1cc6837

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b6ba872Add deprecation.

comment:11 Changed 4 years ago by jmantysalo

Getting back to this one... Is this the right way to start deprecating?

comment:12 follow-up: Changed 4 years ago by tscrim

  • Milestone changed from sage-8.0 to sage-8.1

Almost. You should still have a doctest showing the deprecation.

comment:13 in reply to: ↑ 12 Changed 4 years ago by jmantysalo

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 chapoton

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 git

  • Commit changed from b6ba872027e3f39ad799652625f185d2a1cc6837 to 9e95fc053d6dfa9bdb3dd77fc0dca818cb5cc441

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

9e95fc0Add test for deprecation.

comment:16 Changed 4 years ago by jmantysalo

Strange... I must have tested this without compiling first...

Anyways, now there is a test.

comment:17 follow-up: Changed 4 years ago by tscrim

Triple quote is over-indented. Once fixed, you can set a positive review on my behalf.

comment:18 Changed 4 years ago by git

  • Commit changed from 9e95fc053d6dfa9bdb3dd77fc0dca818cb5cc441 to bca22b361b482130c53e0516fbcb55567a78f2f8

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

bca22b3Indentation of doctest end marker.

comment:19 in reply to: ↑ 17 Changed 4 years ago by jmantysalo

  • Status changed from needs_work to positive_review

Replying to tscrim:

Triple quote is over-indented. Once fixed, you can set a positive review on my behalf.

Done this.

comment:20 Changed 4 years ago by vbraun

  • Branch changed from u/jmantysalo/remove-dist to bca22b361b482130c53e0516fbcb55567a78f2f8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.