py3: fix everything in src/sage/matrix/*

[etn40ff]

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.

[etn40ff]

Last 10 new commits:

​0256751	src/sage/matrix/matrix1.pyx
​dc88750	src/sage/matrix/matrix_double_dense.pyx
​23fbdff	src/sage/matrix/matrix_integer_sparse.pyx
​fc2cc38	src/sage/matrix/matrix_modn_sparse.pyx
​a1c85e2	src/sage/matrix/matrix_rational_sparse.pyx
​9c6825b	src/sage/matrix/matrix_sparse.pyx
​60f1e0b	src/sage/matrix/matrix_modn_dense_template.pxi
​f2b607e	src/sage/graphs/generic_graph.py
​b9075ef	src/sage/matrix/matrix_mod2_dense.pyx
​91a7320	src/sage/matrix/matrix_modn_dense_template.pxi

[git]

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

[jdemeyer]

1. 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)

2. Don't break doctests! You should keep the test A(range(4)), just change it to

   sage: from six.moves import range
   sage: A(range(4))
   

if you want to do the same thing in Python 2 and 3.

[git]

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

​869464d

Reintroduced test

[etn40ff]

Hi Jeroen, thanks for the fast reply.

2. I bowed to your knowledge here and just reintroduced the test.

1. 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 in src/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

[jdemeyer]

Replying to etn40ff:

1. 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.

[jdemeyer]

Also note that isinstance(foo, list) is extremely fast in Cython since there is a particular optimizating in CPython for checking lists.

[tscrim]

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.

[etn40ff]

I am confused. I assumed we do not want to allow this to work if foo is a dictionary, do we?

[tscrim]

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.

[etn40ff]

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)

[jdemeyer]

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?

[etn40ff]

Let me rephrase:

1. 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).

2. With the change I made it preserves this behaviour.

3. 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.

[jdemeyer]

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.

[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

[etn40ff]

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.

[tscrim]

*facepalm*

[tscrim]

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.

[git]

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

[etn40ff]

Dear All, sorry for the late reply.

I made few changes to the code:

1. All instances of [0,1] I introduced in the doctest are now list(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.

2. I removed all the tests isinstance(foo, (Iterator, Sequence)) that where easily removeable. There are few more around, I think only in the following files

   src/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

[jdemeyer]

There is still a lot of list(range(...)) in the doctests.

[etn40ff]

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?

[jdemeyer]

The solution is easy: if the original was A, Travis complains about B and I complain about C, just keep A.

[jdemeyer]

Replying to etn40ff:

#22972

Wow, that's bad indeed. I would never have agreed with that.

[etn40ff]

A is not an option: slicing does not work with iterators and I am not adderssing this issue in this ticket.

[tscrim]

What has happened is you have increased support with changes like this:

src/sage/matrix/matrix1.pyx

@@ -1697 +1697 @@
             [5 4]
             [0 7]
         """
-        if isinstance(columns, xrange):
+        if not isinstance(columns, (list, tuple)):
             columns = list(columns)
-        elif not isinstance(columns, (list, tuple)):
-            raise TypeError("columns (=%s) must be a list of integers" % columns)
+
         cdef Matrix A
         cdef Py_ssize_t ncols,k,r
 

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.

[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?

[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.

[etn40ff]

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

[etn40ff]

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.

[jdemeyer]

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)

[tscrim]

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).

[jdemeyer]

Replying to tscrim:

There is a difference between code that is designed to work with both types of range and those that currently need list(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.

[etn40ff]

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 need list(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.

[jdemeyer]

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.

[chapoton]

here is a modified branch..

---

New commits:

​54645bb

some work on range in matrix folder

[jdemeyer]

Just to remind that this is still needs_work.

[chapoton]

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.

[etn40ff]

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.

[etn40ff]

PS: how is it possible that I am listed as the one making the latest commit on the current branch when I did not?

[chapoton]

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.

[jdemeyer]

1. Why was this doctest removed:

   sage: A.delete_columns('junk')
   

2. 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()

[jdemeyer]

3. For efficiency, it is better to reverse these conditions (replace A and B by B 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.

4. 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.

[jdemeyer]

5. There is still a list(range(3)) in src/sage/matrix/matrix_modn_dense_template.pxi

[jdemeyer]

6. In src/sage/matrix/matrix_modn_sparse.pyx, there is no reason to raise an error for input which is not Iterator, Sequence. Just convert to list as you do in some other places.

[git]

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

​39b799a	Merge branch 'public/23107-v2' in 8.0.b12
​a4bce3b	trac 2310è some work on reviewer's comments

[git]

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

​e2c7587

trac 23107 fixing things

[chapoton]

I think I have taken all of your comments into account. Back to needs review.

[jdemeyer]

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.

[git]

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

​aa3319f

Replace list with generic Exception catch in old parents.

[tscrim]

I took care of it.