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: sage-6.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("cat-says-meow"); P
Posets containing cat-says-meow vertices
sage: P[42]
?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~

Change History (18)

comment:1 Changed 6 years ago by jmantysalo

  • Cc ncohen added

Is there some reason for not to check arguments on other functions? For example

Posets.RestrictedIntegerPartitions('cat-says-meow')
Posets.SymmetricGroupBruhatIntervalPoset('cat-says-meow', 42)
Posets.BooleanLattice('cat-says-meow')

will all give exception, but they output

ValueError: n must be an integer or be equal to one of None, NN,
NonNegativeIntegers()

ValueError: invalid literal for int() with base 10: ''

TypeError: unsupported operand type(s) for ** or pow(): 'int' and 'str'

Is right check just if not n in ZZ: raise...?

comment:2 Changed 6 years ago by ncohen

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 jmantysalo

  • Priority changed from trivial to minor

BooleanLattice(1) return 1-element lattice, not 2-element. 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 jmantysalo

  • Branch set to u/jmantysalo/posets_constructor_does_not_check_argument

comment:5 Changed 6 years ago by jmantysalo

  • Commit set to 335c5462a33e45d40e1d74262212b6b8086f7ecd
  • Status changed from new to needs_review

I copied the check from RandomPoset.


New commits:

335c546Correction for BooleanLattice(n) for n=0 and n=1; some checks for input.

comment:6 Changed 6 years ago by jmantysalo

  • Authors set to Jori Mäntysalo

comment:7 follow-up: Changed 6 years ago by ncohen

  • 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 ; follow-up: Changed 6 years ago by jmantysalo

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 non-negative - -" at RandomPoset(). Of course I should have changed doctest also. But where does this blankline comes?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by ncohen

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 non-negative - -" at RandomPoset(). 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 git

  • Commit changed from 335c5462a33e45d40e1d74262212b6b8086f7ecd to ef5cc86cce07b935b78d68e59b50c2fa78069381

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

ef5cc86Docstring corrected.

comment:11 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by jmantysalo

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 ncohen

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 ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_work to positive_review

Done, and no problem !

Nathann

comment:14 follow-up: Changed 6 years ago by vbraun

  • 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.1-11)] 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 git

  • Commit changed from ef5cc86cce07b935b78d68e59b50c2fa78069381 to a37a626fc2b954ffe9267e86c58b1da6d70c0933

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

a37a626Changed exception messages to incomplete sentences.

comment:16 in reply to: ↑ 14 Changed 6 years ago by jmantysalo

  • Status changed from needs_work to needs_review

Replying to vbraun:

Python style for exeption messages is incomplete sentences.

OK. Corrected.

comment:17 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

Tests pass !

(sorry for the incompatible writing styles...)

Nathann

comment:18 Changed 6 years ago by vbraun

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