Opened 3 years ago

Closed 3 years ago

#22145 closed enhancement (fixed)

LatticePoset: add is_stone

Reported by: jmantysalo Owned by:
Priority: major Milestone: sage-8.0
Component: combinatorics Keywords:
Cc: tscrim Merged in:
Authors: Jori Mäntysalo Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a904582 (Commits) Commit: a904582db50569d31224fa18c1f61ed66f01b60f
Dependencies: Stopgaps:

Description (last modified by jmantysalo)

Add a function to check if a lattice is Stone lattice.

Change History (16)

comment:1 Changed 3 years ago by jmantysalo

  • Branch set to u/jmantysalo/latticeposet__add_is_stone

comment:2 Changed 3 years ago by jmantysalo

  • Commit set to d1a0054cad94bb25f363de87c2a3bc52f038db20
  • Description modified (diff)
  • Milestone changed from sage-7.6 to sage-8.0
  • Status changed from new to needs_review

New commits:

d1a0054Add is_stone().

comment:3 Changed 3 years ago by jmantysalo

  • Cc tscrim added

Travis? Quite trivial one, no complex algorithms.

comment:4 Changed 3 years ago by tscrim

There is a typo Stone algebra and it should be a Stone lattice (the name is being used as an adjective, which is okay, but it still needs an article; the other way would be A lattice is *Stone* if ...).

comment:5 Changed 3 years ago by git

  • Commit changed from d1a0054cad94bb25f363de87c2a3bc52f038db20 to 127c3aa1df361b6253a1687195dc02845ea19184

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

ab65e9aMerge branch 'develop' into t/22145/latticeposet__add_is_stone
127c3aaTypo correction.

comment:6 Changed 3 years ago by jmantysalo

Typo corrected and merged to newest beta.

comment:7 follow-up: Changed 3 years ago by tscrim

You also need the article a Stone lattice in a few places in the is_stone method.

comment:8 Changed 3 years ago by git

  • Commit changed from 127c3aa1df361b6253a1687195dc02845ea19184 to 2a6571b484b104fc1ed96d02579a3ce70de58e9c

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

2a6571bMore typo corrections.

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

Replying to tscrim:

You also need the article a Stone lattice in a few places in the is_stone method.

Done.

comment:10 follow-up: Changed 3 years ago by tscrim

Last two nitpicks.

-            if (sum([x[1] for x in factor(self.cardinality())]) <
-                atoms_n):
+            if sum([x[1] for x in factor(self.cardinality())]) < atoms_n:

and similar. Here it is worthwhile to break the 80 char limit for readability.

Move from sage.arith.misc import factor below the self.is_distributive() check. Actually, I would argue that it should not raise a ValueError in this case, but instead simply return False. However, I've lost similar arguments before (the is_similar of matrices).

comment:11 Changed 3 years ago by git

  • Commit changed from 2a6571b484b104fc1ed96d02579a3ce70de58e9c to a904582db50569d31224fa18c1f61ed66f01b60f

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

3608619Reformat if-clause.
a904582Move import.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by jmantysalo

Replying to tscrim:

Here it is worthwhile to break the 80 char limit for readability.

True, done that.

Move from sage.arith.misc import factor below the self.is_distributive() check.

Well... Done that too. It is of course faster to import only when needed, but it should be a special case if the user tries to see if a non-distributive lattice is a Stone lattice. There are also functions having something like

import somefunction ...
if self.cardinality() < 5: return True
if somefunction(self.bottom()) ...

and I think having imports at a start is more readable.

Actually, I would argue that it should not raise a ValueError in this case, but instead simply return False. However, I've lost similar arguments before (the is_similar of matrices).

Two-sided question. But then at least skeleton() should be changed too.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by tscrim

Replying to jmantysalo:

Replying to tscrim:

Actually, I would argue that it should not raise a ValueError in this case, but instead simply return False. However, I've lost similar arguments before (the is_similar of matrices).

Two-sided question. But then at least skeleton() should be changed too.

skeleton() is a different issue because it is not asking a boolean question, but instead constructing a new object. In that case, the correct thing to do (IMO) is raise a ValueError. While I have a strong opinion on this matter, I don't care enough to refight this battle. So if you prefer this method, then I will set it to a positive review.

comment:14 in reply to: ↑ 13 Changed 3 years ago by jmantysalo

Replying to tscrim:

Replying to jmantysalo:

Replying to tscrim:

Actually, I would argue that it should not raise a ValueError in this case, but instead simply return False. However, I've lost similar arguments before (the is_similar of matrices).

Two-sided question. But then at least skeleton() should be changed too.

skeleton() is a different issue because it is not asking a boolean question, but instead constructing a new object. In that case, the correct thing to do (IMO) is raise a ValueError. While I have a strong opinion on this matter, I don't care enough to refight this battle. So if you prefer this method, then I will set it to a positive review.

I think there is also some other boolean-valued functions with similar behaviour, so please just set to a positive review.

comment:15 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

IMO, that is not a good reason to keep a bad precedent. However, let it be so.

comment:16 Changed 3 years ago by vbraun

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