Opened 3 years ago
Closed 3 years ago
#22145 closed enhancement (fixed)
LatticePoset: add is_stone
Reported by:  jmantysalo  Owned by:  

Priority:  major  Milestone:  sage8.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 )
Add a function to check if a lattice is Stone lattice.
Change History (16)
comment:1 Changed 3 years ago by
 Branch set to u/jmantysalo/latticeposet__add_is_stone
comment:2 Changed 3 years ago by
 Commit set to d1a0054cad94bb25f363de87c2a3bc52f038db20
 Description modified (diff)
 Milestone changed from sage7.6 to sage8.0
 Status changed from new to needs_review
comment:4 Changed 3 years ago by
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
 Commit changed from d1a0054cad94bb25f363de87c2a3bc52f038db20 to 127c3aa1df361b6253a1687195dc02845ea19184
comment:6 Changed 3 years ago by
Typo corrected and merged to newest beta.
comment:7 followup: ↓ 9 Changed 3 years ago by
You also need the article a Stone lattice
in a few places in the is_stone
method.
comment:8 Changed 3 years ago by
 Commit changed from 127c3aa1df361b6253a1687195dc02845ea19184 to 2a6571b484b104fc1ed96d02579a3ce70de58e9c
Branch pushed to git repo; I updated commit sha1. New commits:
2a6571b  More typo corrections.

comment:9 in reply to: ↑ 7 Changed 3 years ago by
Replying to tscrim:
You also need the article
a Stone lattice
in a few places in theis_stone
method.
Done.
comment:10 followup: ↓ 12 Changed 3 years ago by
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
 Commit changed from 2a6571b484b104fc1ed96d02579a3ce70de58e9c to a904582db50569d31224fa18c1f61ed66f01b60f
comment:12 in reply to: ↑ 10 ; followup: ↓ 13 Changed 3 years ago by
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 theself.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 nondistributive 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 import
s at a start is more readable.
Actually, I would argue that it should not raise a
ValueError
in this case, but instead simply returnFalse
. However, I've lost similar arguments before (theis_similar
of matrices).
Twosided question. But then at least skeleton()
should be changed too.
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 3 years ago by
Replying to jmantysalo:
Replying to tscrim:
Actually, I would argue that it should not raise a
ValueError
in this case, but instead simply returnFalse
. However, I've lost similar arguments before (theis_similar
of matrices).Twosided 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
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 returnFalse
. However, I've lost similar arguments before (theis_similar
of matrices).Twosided 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 aValueError
. 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 booleanvalued functions with similar behaviour, so please just set to a positive review.
comment:15 Changed 3 years ago by
 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
 Branch changed from u/jmantysalo/latticeposet__add_is_stone to a904582db50569d31224fa18c1f61ed66f01b60f
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Add is_stone().