rewrite matrix() constructor

[jasongrout]

Currently the code in matrix() is pretty convoluted. This patch is an attempt to make that code much more organized and make it easier to tackle adding features to matrix() and fixing bugs.

Additionally, this patch has quite a few more doctests testing corner casesof matrix().

I am going to run testall on this to make sure that it doesn't mess anything major up. Right now it should pretty much be a drop-in replacement, except that some corner cases are handled more consistently.

Component: linear algebra

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

[malb]

Attachment: sr_matrix.patch.gz

[malb]

comment:2

On [sage-devel] Jason wrote:

More specifically, for sr.py, matrix() is called from lines 1775 and
1779 with a list of lists. The code appears to flatten "l" into a flat
list, but the flatten command on the preceding lines specifies to only
flatten Vector_modn_dense things, so the list "l" is not flattened.

The attached patch sr_matrix.patch should fix that issue.

[jasongrout]

apply instead of previous matrix-refactor.patch patches

[jasongrout]

comment:3

Attachment: matrix-refactor.3.patch.gz

matrix-refactor.3.patch is ready to be reviewed. sr_matrix.patch should be also applied to fix lots of doctests since the new matrix() is a bit stricter on syntax.

There is an issue remaining with transform.pyx (line 44) calling matrix(3,1,elts), where elts is a list containing a RDF vector of 3 elements instead of elts being a list of 3 RDF numbers. I'm opening another ticket for this bug.

[jasongrout]

comment:4

The transform.pyx bug is now #2667.

[jasongrout]

comment:5

One question I have for the patch: Should I be passing back ValueError when I do? Or should it be TypeError or some other error?

[rhinton]

comment:6

This is a good change!

I'm no Python expert, but I like your choice of ValueError over TypeError. I think the paragraph "The entries" in the INPUT: section could be revised a little to improve clarity.

In your example

sage: m=matrix(QQ,3,{(1,1): 2}); m; m.parent() 
[0 0]
[0 2]
[0 0]
...

I expected to see a 3x3 matrix. Your other examples that have only one size input produce square matrices (particularly the empty-dict input argument!). I'm not sure what the "right" behavior is, but I wanted to make sure you are.

I don't see any examples/tests of passing in a Sage object and using the matrix() attribute to get a matrix.

[jasongrout]

comment:7

• the matrix(3,{(1,1): 2}) example is posted to sage-devel for a vote.
  • v=vector; matrix(v) is an example of passing a sage object. We probably ought to have another one, maybe a graph? Do you want to post a quick patch?

sage: g=graphs.CycleGraph(4)
sage: matrix(g)

or something like that, maybe?

[rhinton]

Attachment: matrix-refactor-extra.patch.gz

Apply on top of latest matrix-refactor.patch. Contains doc rewording, extra doctest matrix(graph), redefines matrix(int,dict) according to sage-devel vote.

[rlmill]

comment:8

Note:

Part of the end patches for this ticket should revert the second patch on #2597, since it is a quick fix.

[rhinton]

Apply on top of other patches. Fixes doctests that fail due to semantic change in call to matrix(int, dict). (Semantic change voted +2, -0 on sage-devel.)

[rhinton]

comment:9

Attachment: matrix-refactor-fix-other-tests.patch.gz

Along with the patch for #2667, a "sage -t -long devel/sage/sage" passes with no failures for me. I think this patch is ready as soon as someone can review my changes.

[jasongrout]

comment:10

The patches above all apply cleanly and all doctests (-long) pass if the patch at #2667 is applied as well. I positively review the additional patches (and mine too :). Since rhinton positively reviewed the core patch, I'm going to change the title to positive review.

[jasongrout]

Attachment: trac_2651_vector_comment.patch.gz

Changes a comment to reflect current documentation.

[jasongrout]

comment:11

To clarify, apply all the patches above in order and also apply the patch at #2667.

[dfdeshom]

comment:12

Patch looks good and passes doctests in sage 2.11. This rewrite was much needed!

[sagetrac-mabshoff]

comment:13

Merged all five patches in Sage 3.0.alpha0