Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py3 make doctest of matrix2 future-proof #21630

Closed
fchapoton opened this issue Oct 3, 2016 · 10 comments
Closed

py3 make doctest of matrix2 future-proof #21630

fchapoton opened this issue Oct 3, 2016 · 10 comments

Comments

@fchapoton
Copy link
Contributor

because range will be an iterator in python3

CC: @tscrim

Component: python3

Author: Frédéric Chapoton

Branch/Commit: 55f7277

Reviewer: Jori Mäntysalo

Issue created by migration from https://trac.sagemath.org/ticket/21630

@fchapoton fchapoton added this to the sage-7.4 milestone Oct 3, 2016
@fchapoton
Copy link
Contributor Author

Commit: 55f7277

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/21630

@fchapoton
Copy link
Contributor Author

New commits:

55f7277make doctests of matrix2 future-proof (py3 range)

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Oct 3, 2016

Reviewer: Jori Mäntysalo

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Oct 3, 2016

comment:3

I would vote for this ticket. But maybe I still ask Travis opinion:

This ticket changes matrix(ZZ,3,range(9)) to matrix(ZZ,3,3,range(9)) etc. I think it is good to use examples with both row- and column-count given. Do you agree on that?

(Tests passed etc, so otherwise this is a positive review.)

@tscrim
Copy link
Collaborator

tscrim commented Oct 3, 2016

comment:4

I can understand why this was done, but I think it is good to have the shortcut and used in tests. In particular, it forces us to make updates to the constructor or change the doctests and drop the behavior after a discussion at switch time. I don't have a strong opinion either way for this, but just my 2 cents.

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Oct 3, 2016

comment:5

Is there still left enought of those test with only one dimension given after this patch?

@fchapoton
Copy link
Contributor Author

comment:6

There remains a lot of them, for example in src/sage/matrix/matrix0.pyx

I think we should only keep one such doctest.

EDIT:

git grep -P -c "matrix\([^,0-9]*,[0-9]\s*,\s*range"
src/doc/de/tutorial/introduction.rst:1
src/doc/en/tutorial/introduction.rst:1
src/doc/es/tutorial/introduction.rst:1
src/doc/fr/tutorial/introduction.rst:1
src/doc/ja/tutorial/introduction.rst:1
src/doc/pt/tutorial/introduction.rst:1
src/doc/ru/tutorial/introduction.rst:1
src/sage/interfaces/giac.py:1
src/sage/interfaces/maple.py:2
src/sage/matrix/matrix0.pyx:13
src/sage/matrix/matrix1.pyx:2
src/sage/matrix/matrix2.pyx:13
src/sage/matrix/matrix_double_dense.pyx:15
src/sage/matrix/matrix_integer_dense.pyx:7
src/sage/matrix/matrix_integer_sparse.pyx:2
src/sage/matrix/matrix_rational_dense.pyx:5
src/sage/matrix/matrix_rational_sparse.pyx:4
src/sage/matrix/matrix_real_double_dense.pyx:1
src/sage/modules/matrix_morphism.py:1

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Oct 4, 2016

comment:7

Replying to @fchapoton:

There remains a lot of them, for example in src/sage/matrix/matrix0.pyx

OK then.

I think we should only keep one such doctest.

Yes, maybe. Then it should be on test block and clearly marked what it is for.

git grep -P 

Perl-style regexps... scary.

@vbraun
Copy link
Member

vbraun commented Oct 5, 2016

Changed branch from u/chapoton/21630 to 55f7277

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants