Opened 5 years ago
Closed 5 years ago
#23107 closed enhancement (fixed)
py3: fix everything in src/sage/matrix/*
Reported by: | etn40ff | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.0 |
Component: | python3 | Keywords: | |
Cc: | chapoton | Merged in: | |
Authors: | Salvatore Stella | Reviewers: | Jeroen Demeyer, Travis Scrimshaw, Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | aa3319f (Commits, GitHub, GitLab) | Commit: | aa3319f7a7a92f3b5507f7d11f4a7c0d8ff0a49b |
Dependencies: | Stopgaps: |
Description
Part of #16081
This set of changes takes care of all the problems reported by sage -t --long src/sage/matrix/*
. With probability 1 there are other functions that would need to be changed as well but that are not tested enough to raise errors at the moment.
It is my first ticket concerning pythn3 so please be extra careful when reviewing it.
This marginally changed also src/sage/graphs/generic_graph.py
.
Change History (55)
comment:1 Changed 5 years ago by
- Branch set to public/ticket/23107
- Commit set to 91a7320801d289bd2a57e26ef14df0d56d696f84
comment:2 Changed 5 years ago by
- Status changed from new to needs_review
comment:3 Changed 5 years ago by
- Type changed from PLEASE CHANGE to enhancement
comment:4 Changed 5 years ago by
- Commit changed from 91a7320801d289bd2a57e26ef14df0d56d696f84 to 2108e1bac5d6a46a05b8165b39f96c1fbbc3ef4c
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
b286036 | src/sage/matrix/matrix1.pyx
|
b3d427f | src/sage/matrix/matrix_double_dense.pyx
|
8c31595 | src/sage/matrix/matrix_integer_sparse.pyx
|
f96bfc6 | src/sage/matrix/matrix_modn_sparse.pyx
|
4521f1d | src/sage/matrix/matrix_rational_sparse.pyx
|
970bde8 | src/sage/matrix/matrix_sparse.pyx
|
a015c6f | src/sage/matrix/matrix_modn_dense_template.pxi
|
4e8346f | src/sage/graphs/generic_graph.py
|
f51aaf6 | src/sage/matrix/matrix_mod2_dense.pyx
|
2108e1b | src/sage/matrix/matrix_modn_dense_template.pxi
|
comment:5 Changed 5 years ago by
- Status changed from needs_review to needs_work
- I would replace
from collections import Iterator, Sequence if isinstance(columns, (Iterator, Sequence)): columns = list(columns) else: raise TypeError("columns (=%s) must be a list of integers" % columns)
by
columns = list(columns)
- Don't break doctests! You should keep the test
A(range(4))
, just change it tosage: from six.moves import range sage: A(range(4))
if you want to do the same thing in Python 2 and 3.
comment:6 Changed 5 years ago by
- Commit changed from 2108e1bac5d6a46a05b8165b39f96c1fbbc3ef4c to 869464dc611b299e8c28794470277d6ed399e903
Branch pushed to git repo; I updated commit sha1. New commits:
869464d | Reintroduced test
|
comment:7 follow-up: ↓ 8 Changed 5 years ago by
Hi Jeroen, thanks for the fast reply.
- I bowed to your knowledge here and just reintroduced the test.
- The rationale behind the conditional clause you mention (and the many other similar occurrences I added in this partch) is that, most likely,
columns
is already going to be a list. I figured it was more efficient to add two checks rather than recreating a new list blindly. In this specific case you mention it will probably not matter because these lists will have short lengths anyway. The cases in which this is more relevant are probably insrc/sage/matrix/matrix_integer_sparse.pyx
where, in principle, one could be building a very big matrix. Should I remove these tests?
One other thing: before settling on this form I was just checking for hasattr(columns, "__iter__")
but then I realized that also dictionaries will satisfy this.
New commits:
869464d | Reintroduced test
|
comment:8 in reply to: ↑ 7 Changed 5 years ago by
Replying to etn40ff:
- The rationale behind the conditional clause you mention (and the many other similar occurrences I added in this partch) is that, most likely,
columns
is already going to be a list.
This reply seems unrelated to the comment I made. It's not about calling list()
when the input is not a list, it's about rejecting things which are not a Sequence
or Iterator
.
I'm perfectly fine with code like
if not isinstance(foo, list): foo = list(foo)
As you say, this is more efficient if the input is already a list
.
comment:9 Changed 5 years ago by
Also note that isinstance(foo, list)
is extremely fast in Cython since there is a particular optimizating in CPython for checking lists.
comment:10 Changed 5 years ago by
I agree with Jeroen, you want to construct a list and let Python fail when list(foo)
doesn't work. We shouldn't force it to be either an Iterator
or Sequence
, and fail otherwise. This would allow things like the Python3 range
to be valid input. Plus it could get rid of the (relatively) expensive from collections import Iterator, Sequence
.
comment:11 follow-up: ↓ 14 Changed 5 years ago by
I am confused. I assumed we do not want to allow this to work if foo
is a dictionary, do we?
comment:12 Changed 5 years ago by
Then we can explicitly forbid that, but I think this would break Python's ducktyping principle. Since it is suppose to take any iterable, and a dict
is an iterable, it should work. A user passing a dict
is likely an error on the user's part, but there could be a use case for it.
comment:13 Changed 5 years ago by
OK, I consider myself voted down and I will remove these checks.
I would only like to point out that we are changing the behaviour from the one before this patch.
The current patch:
- if isinstance(columns, xrange): - columns = list(columns) - elif not isinstance(columns, (list, tuple)): - raise TypeError("columns (=%s) must be a list of integers" % columns) + if not isinstance(columns, (list, tuple)): + from collections import Iterator, Sequence + if isinstance(columns, (Iterator, Sequence)): + columns = list(columns) + else: + raise TypeError("columns (=%s) must be a list of integers" % columns)
The proposed version:
- if isinstance(columns, xrange): - columns = list(columns) - elif not isinstance(columns, (list, tuple)): - raise TypeError("columns (=%s) must be a list of integers" % columns) + if not isinstance(columns, (list, tuple)): + columns = list(columns)
comment:14 in reply to: ↑ 11 Changed 5 years ago by
Replying to etn40ff:
I assumed we do not want to allow this to work if
foo
is a dictionary, do we?
Why should we explicitly disallow a dict
?
comment:15 follow-up: ↓ 16 Changed 5 years ago by
Let me rephrase:
- With the code in sage before this ticket was opened the function in the comment 13 fails if you pass a dictionary to it (or any other iterable that is not xrange list or tuple).
- With the change I made it preserves this behaviour.
- With the change you suggest it works also if
coulmns
is a dictionary (using the its keys as elements).
I am not saying that 3 is bad (I actually prefer it), it is just different from 1.
comment:16 in reply to: ↑ 15 Changed 5 years ago by
Replying to etn40ff:
I am not saying that 3 is bad (I actually prefer it), it is just different from 1.
Well, you are already changing the behaviour of the function in other ways too.
If allowing dicts was the only change that you did, I would agree with you: that would be dubious. But if you change behaviour anyway, then the argument "it behaves different from the old code" makes little sense.
comment:17 follow-up: ↓ 18 Changed 5 years ago by
Something that is a problem with your current version is that it no longer would support xrange
:
sage: from six.moves import range sage: isinstance(range, Iterator) False sage: isinstance(range, Sequence) False
comment:18 in reply to: ↑ 17 Changed 5 years ago by
Replying to tscrim:
Something that is a problem with your current version is that it no longer would support
xrange
:sage: from six.moves import range sage: isinstance(range, Iterator) False sage: isinstance(range, Sequence) False
That is not surprising: you forgot to call it.
sage: from six.moves import range sage: isinstance(range(10), Iterator) False sage: isinstance(range(10), Sequence) True
Anyway this is a moot point now since we decided that I should remove these conditional statements.
comment:19 Changed 5 years ago by
*facepalm*
comment:20 Changed 5 years ago by
- Reviewers set to Jeroen Demeyer, Travis Scrimshaw
Can you revert some (all?) of the changes from range
to explicit lists and be Python3 compatible? I think we should have coverage for (at least most) those as features. Once that is done, I think that takes care of all of the issues and we can set this to a positive review.
Also, author name.
comment:21 Changed 5 years ago by
- Commit changed from 869464dc611b299e8c28794470277d6ed399e903 to 8a5b84949339869d42091328874bd8ac83ab45d7
Branch pushed to git repo; I updated commit sha1. New commits:
e2749d7 | Merge branch 'public/ticket/23107' of git://trac.sagemath.org/sage into public/ticket/23107
|
5101129 | Revert to lists in src/sage/matrix/docs.py
|
c252f99 | Revert to lists in src/sage/matrix/matrix0.pyx
|
7c474a9 | Removed iterator tests from src/sage/matrix/matrix1.pyx
|
454671f | Removed iterator tests from src/sage/matrix/matrix1.pyx (bis)
|
6138023 | Revert to lists in src/sage/matrix/matrix_complex_ball_dense.pyx
|
0c6c2ca | Revert to lists in src/sage/matrix/matrix_integer_dense.pyx
|
af55f07 | Removed iterator test from src/sage/matrix/matrix_sparse.pyx
|
efa0b9d | typo
|
8a5b849 | Missing list
|
comment:22 Changed 5 years ago by
Dear All, sorry for the late reply.
I made few changes to the code:
- All instances of
[0,1]
I introduced in the doctest are nowlist(range(2))
. Same for[0,1,2,3]
. I do not understand why you wanted this change (they produce the same output both in python2 and python3 and the fisrt syntax is shorter and more legible IMHO) but it is not worth spending time discussing this.
- I removed all the tests
isinstance(foo, (Iterator, Sequence))
that where easily removeable. There are few more around, I think only in the following filessrc/sage/matrix/matrix_double_dense.pyx src/sage/matrix/matrix_integer_sparse.pyx src/sage/matrix/matrix_mod2_dense.pyx src/sage/matrix/matrix_modn_sparse.pyx src/sage/matrix/matrix_rational_sparse.pyx src/sage/matrix/matrix_space.py
The reason is that the function in there already accept a variety of inputs, some of which are not iterable, and to remove the tests these function would have to be significantly reworked. There are two solutions to this:
(cheap and messy) remove the test and fail if they are fed an iterator. This probably will require to fix doctests in other files. Plus I do not see why they should not work with iterators just because the implementation is not clean enough.
(more time consuming) rewrite the functions so that they default to evaluate
foo = list(foo)
At the moment I do not have time to do so. If you are not satisfied feel free to take over this patch and fix it to your likings.
@Travis: I do not understand what you mean by "and be Python3 compatible". It looks to me that the changes this patch produces work in both version of python. This is the whole point, no?
New commits:
e2749d7 | Merge branch 'public/ticket/23107' of git://trac.sagemath.org/sage into public/ticket/23107
|
5101129 | Revert to lists in src/sage/matrix/docs.py
|
c252f99 | Revert to lists in src/sage/matrix/matrix0.pyx
|
7c474a9 | Removed iterator tests from src/sage/matrix/matrix1.pyx
|
454671f | Removed iterator tests from src/sage/matrix/matrix1.pyx (bis)
|
6138023 | Revert to lists in src/sage/matrix/matrix_complex_ball_dense.pyx
|
0c6c2ca | Revert to lists in src/sage/matrix/matrix_integer_dense.pyx
|
af55f07 | Removed iterator test from src/sage/matrix/matrix_sparse.pyx
|
efa0b9d | typo
|
8a5b849 | Missing list
|
New commits:
e2749d7 | Merge branch 'public/ticket/23107' of git://trac.sagemath.org/sage into public/ticket/23107
|
5101129 | Revert to lists in src/sage/matrix/docs.py
|
c252f99 | Revert to lists in src/sage/matrix/matrix0.pyx
|
7c474a9 | Removed iterator tests from src/sage/matrix/matrix1.pyx
|
454671f | Removed iterator tests from src/sage/matrix/matrix1.pyx (bis)
|
6138023 | Revert to lists in src/sage/matrix/matrix_complex_ball_dense.pyx
|
0c6c2ca | Revert to lists in src/sage/matrix/matrix_integer_dense.pyx
|
af55f07 | Removed iterator test from src/sage/matrix/matrix_sparse.pyx
|
efa0b9d | typo
|
8a5b849 | Missing list
|
comment:23 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:24 Changed 5 years ago by
- Status changed from needs_review to needs_work
There is still a lot of list(range(...))
in the doctests.
comment:25 follow-up: ↓ 27 Changed 5 years ago by
I am totally confused here. The code contained many instances of things like M[range(2):]
and I changed that to M[[0,1],:]
. Travis complained in comment 20 and I reverted them to M[list(range(2)):]
which is the way things have been done in other tickets (c.f. #22972 https://git.sagemath.org/sage.git/commitid=5c740530b0224aab62eaa8e4e49d0dfd9a19ec08 ). Now you complain for the exact opposite reason. Can we please reach an agreement on what should be done?
comment:26 Changed 5 years ago by
The solution is easy: if the original was A, Travis complains about B and I complain about C, just keep A.
comment:27 in reply to: ↑ 25 Changed 5 years ago by
comment:28 follow-up: ↓ 30 Changed 5 years ago by
A is not an option: slicing does not work with iterators and I am not adderssing this issue in this ticket.
comment:29 Changed 5 years ago by
What has happened is you have increased support with changes like this:
-
src/sage/matrix/matrix1.pyx
diff --git a/src/sage/matrix/matrix1.pyx b/src/sage/matrix/matrix1.pyx index b9c35be..dfe74a9 100644
a b cdef class Matrix(matrix0.Matrix): 1697 1697 [5 4] 1698 1698 [0 7] 1699 1699 """ 1700 if isinstance(columns, xrange):1700 if not isinstance(columns, (list, tuple)): 1701 1701 columns = list(columns) 1702 elif not isinstance(columns, (list, tuple)): 1703 raise TypeError("columns (=%s) must be a list of integers" % columns) 1702 1704 1703 cdef Matrix A 1705 1704 cdef Py_ssize_t ncols,k,r 1706 1705
and you do not need to explicitly specify it as a list. By allowing an xrange
(which is Cython-speak for Python3 range) input, you are Python3 compatible. So I don't fundamentally see my and Jeroen's objections as being different (actually, mine was more meant as a question at the time).
Edit: Misread the code, it is not a regression but an expansion of support.
comment:30 in reply to: ↑ 28 ; follow-up: ↓ 32 Changed 5 years ago by
Replying to etn40ff:
A is not an option: slicing does not work with iterators and I am not adderssing this issue in this ticket.
Sorry, I don't understand what you mean. Why cannot you just keep the doctests as they are?
comment:31 follow-up: ↓ 33 Changed 5 years ago by
There is more general idea here: do not break doctests. If you make changes to the code which break a doctest, that's bad: you should fix your code such that the doctest remains to work.
There are always exceptions to this rule, but you need good reasons.
comment:32 in reply to: ↑ 30 Changed 5 years ago by
Replying to jdemeyer:
Replying to etn40ff:
A is not an option: slicing does not work with iterators and I am not adderssing this issue in this ticket.
Sorry, I don't understand what you mean. Why cannot you just keep the doctests as they are?
Because of this:
sage: M = Matrix(5); M[iter(range(3)):] --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-4-ca1bbd50e7c9> in <module>() ----> 1 M = Matrix(Integer(5)); M[iter(range(Integer(3))):] /opt/sage/src/sage/matrix/matrix0.pyx in sage.matrix.matrix0.Matrix.__getitem__ (build/cythonized/sage/matrix/matrix0.c:7659)() 984 r = self.matrix_from_rows(row_list) 985 elif isinstance(row_index, slice): --> 986 row_list = list(xrange(*row_index.indices(nrows))) 987 r = self.matrix_from_rows(row_list) 988 else: TypeError: slice indices must be integers or None or have an __index__ method
comment:33 in reply to: ↑ 31 ; follow-up: ↓ 34 Changed 5 years ago by
Replying to jdemeyer:
There is more general idea here: do not break doctests. If you make changes to the code which break a doctest, that's bad: you should fix your code such that the doctest remains to work.
There are always exceptions to this rule, but you need good reasons.
I *did not* break a doctest. I have been applying the same procedure taht all the other python3 tickets did i.e. adapt the doctest to accept both python2 and python3. I agree that list(range(
is fugly but that is what has been done all over the place.
comment:34 in reply to: ↑ 33 Changed 5 years ago by
Replying to etn40ff:
i.e. adapt the doctest to accept both python2 and python3
Adapting doctests is not a solution. The change from Python 2 to Python 3 should be as transparent as possible to the user. You cannot require the user to change all range(n)
into list(range(n))
.
(edit: as usual, there are exceptions. Some things cannot work in Python 3 for fundamental reasons. But I see no reason to make an exception for this ticket)
comment:35 follow-up: ↓ 36 Changed 5 years ago by
There is a difference between code that is designed to work with both types of range
and those that currently need list(range(
. Although perhaps on earlier Python3 tickets, Frédéric and I didn't quite know what the best way forward would be (such as Cython's support for xrange
).
comment:36 in reply to: ↑ 35 ; follow-up: ↓ 37 Changed 5 years ago by
comment:37 in reply to: ↑ 36 ; follow-up: ↓ 38 Changed 5 years ago by
- Cc chapoton added
Replying to jdemeyer:
Replying to tscrim:
There is a difference between code that is designed to work with both types of
range
and those that currently needlist(range(
.As I explained in 34, the solution is to change the code to accept
range(n)
in Python 3. Changing doctests is not a solution.
This patch does not address the issue of slicing; are you saying that it should leave alone the doctests failing because of this?
In any case, it looks to me that there is no consensus on what to do. Moreover we are not being consistent with other py3 tickets so far. Do we have to revisit all the work that has already been done? Frédéric should probably be part of the conversation as well so I just cced him.
I guess that me trying to simultaneously satisfy contrasting guidelines is pointless so I will step back and let you figure out a list of desiderata first. I do not think I will have much time to work on this in the foreseeable future (I am moving to another country for my next job) so please feel free to claim ownership of this ticket and continue without me.
comment:38 in reply to: ↑ 37 Changed 5 years ago by
Replying to etn40ff:
are you saying that it should leave alone the doctests failing because of this?
Exactly (to be clear, I assume that by "failing" you mean "failing on Python 3 but succeeding on Python 2").
Moreover we are not being consistent with other py3 tickets so far.
That's a pity indeed. Note that most of the Python 3 tickets do not change doctests that way. #22972 is the only one that I am aware of (and to repeat, I disagree with what was done there).
Do we have to revisit all the work that has already been done?
Well, in any case that's not really urgent. But at least let's not make things worse.
comment:39 Changed 5 years ago by
- Branch changed from public/ticket/23107 to public/23107-v2
- Commit changed from 8a5b84949339869d42091328874bd8ac83ab45d7 to 54645bb8c4320a2a0b685771c3e4e69582cde286
comment:40 Changed 5 years ago by
Just to remind that this is still needs_work.
comment:41 Changed 5 years ago by
- Status changed from needs_work to needs_review
Thanks. I think Salvatore has dropped the case. I would have liked to see this resolved without me doing anything, but alas..
I am not totally convinced by the use of Iterator, Sequence.
comment:42 Changed 5 years ago by
Dear all, I just wanted to say that I did not drop the cause yet. I recently moved to my new job and I am still on a crazy rollercoaster trying to sort things out. If you are not in a hurry I can be back on this in a month time at the latest. On the other hand, if you are in a hurry feel free to push through without my input. There are going to be plenty of other py3 tickets I can contribute to once I am back.
Sorry for not being more helpful right now.
comment:43 Changed 5 years ago by
PS: how is it possible that I am listed as the one making the latest commit on the current branch when I did not?
comment:44 Changed 5 years ago by
Hello Salvatore,
in fact, there is no hurry (py3 is still a long way ahead, sadly), but I was afraid that you were (besides being very busy with installation at your new job, which is very understandable) somewhat horrified by the complexity of the matter, and the various contradictory requirements.
Concerning your name on the commit, I did a rebase, keeping only your first commit and squashing the others into this one, then changing its commit message. Then I worked a little bit and rebased again. So I guess this is considered to some kind of "transformation" of your initial commit.
comment:45 Changed 5 years ago by
- Status changed from needs_review to needs_work
- Why was this doctest removed:
sage: A.delete_columns('junk')
- This might be partially a style issue, but I hate code of the form
try: something() except TypeError: raise TypeError(...)
because usually, the exception that is caught and thrown away contains more useful information that the newly raised exception. So I would replace this by
something()
comment:46 Changed 5 years ago by
- For efficiency, it is better to reverse these conditions (replace
A and B
byB and A
):if (isinstance(x, (Iterator, Sequence)) and not isinstance(x, (list, tuple))):
First of all, it's much cheaper to check if something is a list
or tuple
rather than some ABC type. Second, in many cases, the input will be a list
or tuple
, so then you don't even need the (Iterator, Sequence)
check.
- Again for efficiency, move all imports of
Iterator, Sequence
to the top level, not inside the function. Since these are standard Python types, there should be no harm.
comment:47 Changed 5 years ago by
- There is still a
list(range(3))
insrc/sage/matrix/matrix_modn_dense_template.pxi
comment:48 Changed 5 years ago by
- In
src/sage/matrix/matrix_modn_sparse.pyx
, there is no reason to raise an error for input which is notIterator, Sequence
. Just convert tolist
as you do in some other places.
comment:49 Changed 5 years ago by
- Commit changed from 54645bb8c4320a2a0b685771c3e4e69582cde286 to a4bce3b1dae3d961a5ed301fa37e287f0195a745
comment:50 Changed 5 years ago by
- Commit changed from a4bce3b1dae3d961a5ed301fa37e287f0195a745 to e2c758798bf8a5d873f9b3b6020706ed55f81e0d
Branch pushed to git repo; I updated commit sha1. New commits:
e2c7587 | trac 23107 fixing things
|
comment:51 Changed 5 years ago by
- Status changed from needs_work to needs_review
I think I have taken all of your comments into account. Back to needs review.
comment:52 Changed 5 years ago by
- Status changed from needs_review to needs_work
In parent_old.pyx
, better give up on trying to list all possible exceptions. If you replace that by except Exception
, you can set this to positive review.
comment:53 Changed 5 years ago by
- Commit changed from e2c758798bf8a5d873f9b3b6020706ed55f81e0d to aa3319f7a7a92f3b5507f7d11f4a7c0d8ff0a49b
Branch pushed to git repo; I updated commit sha1. New commits:
aa3319f | Replace list with generic Exception catch in old parents.
|
comment:54 Changed 5 years ago by
- Reviewers changed from Jeroen Demeyer, Travis Scrimshaw to Jeroen Demeyer, Travis Scrimshaw, Frédéric Chapoton
- Status changed from needs_work to positive_review
I took care of it.
comment:55 Changed 5 years ago by
- Branch changed from public/23107-v2 to aa3319f7a7a92f3b5507f7d11f4a7c0d8ff0a49b
- Resolution set to fixed
- Status changed from positive_review to closed
Last 10 new commits:
src/sage/matrix/matrix1.pyx
src/sage/matrix/matrix_double_dense.pyx
src/sage/matrix/matrix_integer_sparse.pyx
src/sage/matrix/matrix_modn_sparse.pyx
src/sage/matrix/matrix_rational_sparse.pyx
src/sage/matrix/matrix_sparse.pyx
src/sage/matrix/matrix_modn_dense_template.pxi
src/sage/graphs/generic_graph.py
src/sage/matrix/matrix_mod2_dense.pyx
src/sage/matrix/matrix_modn_dense_template.pxi