Opened 6 years ago
Closed 6 years ago
#17129 closed defect (fixed)
Posets constructor does not check argument
Reported by:  jmantysalo  Owned by:  

Priority:  minor  Milestone:  sage6.4 
Component:  combinatorics  Keywords:  
Cc:  ncohen  Merged in:  
Authors:  Jori Mäntysalo  Reviewers:  Nathann Cohen 
Report Upstream:  N/A  Work issues:  
Branch:  a37a626 (Commits)  Commit:  a37a626fc2b954ffe9267e86c58b1da6d70c0933 
Dependencies:  Stopgaps: 
Description
sage: P=Posets("catsaysmeow"); P Posets containing catsaysmeow vertices sage: P[42] ?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{}~
Change History (18)
comment:1 Changed 6 years ago by
 Cc ncohen added
comment:2 Changed 6 years ago by
Well, as long as an exception is raised I do not mind much, personally. Though you can add an explicit check if it helps you sleep better :P
Nathann
comment:3 Changed 6 years ago by
 Priority changed from trivial to minor
BooleanLattice(1)
return 1element lattice, not 2element. Hence I raise this from trivial level to minor. (And write this comment as a note to myself.)
comment:4 Changed 6 years ago by
 Branch set to u/jmantysalo/posets_constructor_does_not_check_argument
comment:5 Changed 6 years ago by
 Commit set to 335c5462a33e45d40e1d74262212b6b8086f7ecd
 Status changed from new to needs_review
I copied the check from RandomPoset
.
New commits:
335c546  Correction for BooleanLattice(n) for n=0 and n=1; some checks for input.

comment:6 Changed 6 years ago by
comment:7 followup: ↓ 8 Changed 6 years ago by
 Status changed from needs_review to needs_work
Hello!
It looks good but you should check the doctests. There are errors when you run them in the poset folder.
Nathann
comment:8 in reply to: ↑ 7 ; followup: ↓ 9 Changed 6 years ago by
Replying to ncohen:
It looks good but you should check the doctests. There are errors when you run them in the poset folder.
I do not understand. It says Got: <BLANKLINE>
. Only change I did was changing "numbers" to "Numbers" in phrase "number of elements must be nonnegative  " at RandomPoset()
. Of course I should have changed doctest also. But where does this blankline comes?
comment:9 in reply to: ↑ 8 ; followup: ↓ 11 Changed 6 years ago by
Yo !
I do not understand. It says
Got: <BLANKLINE>
. Only change I did was changing "numbers" to "Numbers" in phrase "number of elements must be nonnegative  " atRandomPoset()
. Of course I should have changed doctest also. But where does this blankline comes?
The blankline is a bluff. You did not only change the n>N
but also the final '.'.
Sphinx is not the kind of software that you can trust to give meaningful error messages ^^;
Nathann
comment:10 Changed 6 years ago by
 Commit changed from 335c5462a33e45d40e1d74262212b6b8086f7ecd to ef5cc86cce07b935b78d68e59b50c2fa78069381
Branch pushed to git repo; I updated commit sha1. New commits:
ef5cc86  Docstring corrected.

comment:11 in reply to: ↑ 9 ; followup: ↓ 12 Changed 6 years ago by
Replying to ncohen:
Sphinx is not the kind of software that you can trust to give meaningful error messages
^^;
Grrrr....
Thanks, corrected this, also added another dots.
comment:12 in reply to: ↑ 11 Changed 6 years ago by
Thanks, corrected this, also added another dots.
Okay ! I will run all of Sage's doctests because the Poset code is called in weird places, then will switch change the ticket's status.
Nathann
comment:13 Changed 6 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_work to positive_review
Done, and no problem !
Nathann
comment:14 followup: ↓ 16 Changed 6 years ago by
 Status changed from positive_review to needs_work
Python style for exeption messages is incomplete sentences. In particular, no capitalization of the first word and no period at the end. E.g.
$ python Python 2.7.8 (default, Oct 27 2014, 15:54:28) [GCC 4.9.1 20140930 (Red Hat 4.9.111)] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> 1 + '2' Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: unsupported operand type(s) for +: 'int' and 'str'
This ticket makes it worse in many places.
comment:15 Changed 6 years ago by
 Commit changed from ef5cc86cce07b935b78d68e59b50c2fa78069381 to a37a626fc2b954ffe9267e86c58b1da6d70c0933
Branch pushed to git repo; I updated commit sha1. New commits:
a37a626  Changed exception messages to incomplete sentences.

comment:16 in reply to: ↑ 14 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:17 Changed 6 years ago by
 Status changed from needs_review to positive_review
Tests pass !
(sorry for the incompatible writing styles...)
Nathann
comment:18 Changed 6 years ago by
 Branch changed from u/jmantysalo/posets_constructor_does_not_check_argument to a37a626fc2b954ffe9267e86c58b1da6d70c0933
 Resolution set to fixed
 Status changed from positive_review to closed
Is there some reason for not to check arguments on other functions? For example
will all give exception, but they output
Is right check just
if not n in ZZ: raise...
?