Opened 6 years ago
Closed 6 years ago
#18959 closed enhancement (fixed)
Poset documentation polishing: integervalued properties
Reported by:  jmantysalo  Owned by:  

Priority:  minor  Milestone:  sage6.9 
Component:  documentation  Keywords:  
Cc:  ncohen, vdelecroix  Merged in:  
Authors:  Jori Mäntysalo  Reviewers:  Nathann Cohen 
Report Upstream:  N/A  Work issues:  
Branch:  3302f91 (Commits, GitHub, GitLab)  Commit:  3302f91538b7258d2ad58d413c84a69e0308a53c 
Dependencies:  Stopgaps: 
Description (last modified by )
Change History (14)
comment:1 Changed 6 years ago by
 Branch set to u/jmantysalo/poset_documentation_polishing__integer_valued_properties
comment:2 Changed 6 years ago by
 Cc ncohen added
 Commit set to 1a21f1f475656fa62525f4acef7790c0857bd963
comment:3 followup: ↓ 4 Changed 6 years ago by
 Cc vdelecroix added
Vincent will probably be interested by you int>Integer change.
comment:4 in reply to: ↑ 3 ; followup: ↓ 5 Changed 6 years ago by
Replying to ncohen:
Vincent will probably be interested by you int>Integer change.
OK. I am not sure what way this should be. For now return type of poset cardinality()
differs from type of graph order()
.
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 6 years ago by
OK. I am not sure what way this should be. For now return type of poset
cardinality()
differs from type of graphorder()
.
This inconsistency was probably introduced by #18159.
comment:6 in reply to: ↑ 5 Changed 6 years ago by
Replying to ncohen:
OK. I am not sure what way this should be. For now return type of poset
cardinality()
differs from type of graphorder()
.This inconsistency was probably introduced by #18159.
OK, so cardinality()
can be seen as an exception. But it does not explain why width()
and height()
gives different return types.
comment:7 followup: ↓ 8 Changed 6 years ago by
There is ongoing discussion about int
vs. Integer
in both sagedevel and in another ticket. Nathann, just to make sure: I guess you have nothing against changing return type of height()
and dimension()
?
You might be interested in other changes too, as I am touching functions made by you.
comment:8 in reply to: ↑ 7 ; followup: ↓ 10 Changed 6 years ago by
There is ongoing discussion about
int
vs.Integer
in both sagedevel and in another ticket. Nathann, just to make sure: I guess you have nothing against changing return type ofheight()
anddimension()
?
You guess wrong. I view it as the progressive suppression of 'integers' from Sage, for absolutely no good reason. It will make us import Integer everywhere, and wrap infinitely many 'return x' with 'return Integer(x)', just because people cannot stand that Sage's integer is not the default one everywhere.
There is a discussion on #18159 on the theme "I don't mind your returning Sage Integers if you like, but I don't see why you want to force me to do it too".
Nathann
comment:9 Changed 6 years ago by
 Commit changed from 1a21f1f475656fa62525f4acef7790c0857bd963 to 2b190b2de01c21578efefe5d876f83e330a12197
Branch pushed to git repo; I updated commit sha1. New commits:
2b190b2  Integer>int, minor other changes.

comment:10 in reply to: ↑ 8 Changed 6 years ago by
 Status changed from new to needs_review
Replying to ncohen:
There is ongoing discussion about
int
vs.Integer
in both sagedevel and in another ticket. Nathann, just to make sure: I guess you have nothing against changing return type ofheight()
anddimension()
?You guess wrong.
OK, here is modified patch; int
vs. Integer
can be resolved later.
This patch contains nonrelating modification to random_order_ideal()
: direction='junk'
will now raise exception instead of behaving like direction='up'
.
comment:11 followup: ↓ 13 Changed 6 years ago by
 Reviewers set to Nathann Cohen
Hello !
Two comments:
 There is an 'Integer(0)' left
 It is (slightly) better to have
# random
than# not tested
: in the first case the command is not run, while in the second it is run and the output is not compared with the expected results (which would still report an error in case of an exception). Given the command in question ('sage: L') it hardly matters here
Regardless of what you decide to do with those two points, you can set the ticket to positive_review
on my behalf.
Nathann
comment:12 Changed 6 years ago by
 Commit changed from 2b190b2de01c21578efefe5d876f83e330a12197 to 3302f91538b7258d2ad58d413c84a69e0308a53c
Branch pushed to git repo; I updated commit sha1. New commits:
3302f91  Forgotten Integer(0)

comment:13 in reply to: ↑ 11 Changed 6 years ago by
 Description modified (diff)
 Status changed from needs_review to positive_review
Replying to ncohen:
 It is (slightly) better to have
# random
than# not tested
: in the first case the command is not run  
True. Corrected.
 There is an 'Integer(0)' left
Regardless of what you decide to do with those two points, you can set the ticket to
positive_review
on my behalf.
I would have said this a bug. I corrected it also.
Thanks!
comment:14 Changed 6 years ago by
 Branch changed from u/jmantysalo/poset_documentation_polishing__integer_valued_properties to 3302f91538b7258d2ad58d413c84a69e0308a53c
 Resolution set to fixed
 Status changed from positive_review to closed
Code changes: Return type of all functions is now
Integer
. There was slight inconsistency between for exampledimension()
andwidth()
.Tests: Added test for empty poset to those functions where it wasn't already.
Docstrings: Added some some kind of "humanreadable" explanation to
dimension()
. Otherwise normal things like "returns" > "return".Examples: Tried to make simple examples that are possible to verify by hand. For example
relations_number
for the Pentagon Poset. Fordimension()
selected a beatiful poset (with untrivial dimension).:=)
Questions:
Is it exactly true that "Relations are also often called intervals."? I would say that every relation (i.e. pair of comparable elemenst) *defines* an interval.
Is the term 'realizer' commonly used? If so, it should be mentioned in the documentation of
dimension()
(because of googling).There is one 'not tested' in
dimension()
. Can a test combine 'long time' and optional? As it takes less than 30 seconds with additional package, it could be tested withlong
.About English, should one say "the longest chain" or "a longest chain" when there can be several chains equal in lenght? (Compare to "a smallest set of linear extensions" on
dimension()
.)New commits:
Some polishing to integervalued poset properties.