block_matrix reacts inconsistently with 0

[sagetrac-jbmohler]

Using ZZ(0) as an element of the list passed to block_matrix appears to be a special case somehow and throws an exception rather than creating the matrix seems reasonable to me.

sage: i=MatrixSpace(ZZ,2,2)(1)
sage: i

[1 0]
[0 1]
sage: block_matrix([1,i,1,1])  # this works as I expect

[1 0|1 0]
[0 1|0 1]
[---+---]
[1 0|1 0]
[0 1|0 1]
sage: block_matrix([0,i,1,1])  # this doesn't ... why is 0 special
...
ValueError: Insufficient information to determine dimensions.

This feels to me like a hazardous inconsistency.

Perhaps I should also add that I don't really like that it just blithely assumes I want a square matrix (although I did in my actual usage). Ticket #2429 addresses that issue more wholeheartedly.

Apply:

1. attachment: 4492_block_matrix_rebased.patch
2. attachment: trac_4492-block-matrix-reviewer.patch
3. attachment: 4492_typo.patch
4. attachment: trac_4492-doctest-number-field.patch

CC: @craigcitro @jasongrout @loefflerd

Component: linear algebra

Author: Willem Jan Palenstijn

Reviewer: Aly Deines, Rob Beezer

Merged: sage-4.6.2.alpha3

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

[sagetrac-sdietzel]

comment:4

So I figured out what is going wrong here. Patch to come shortly.

[wjp]

comment:6

I think that in the doctest you give there actually is insufficient information, because you can't deduce the width of the left blocks, so an exception is in my opinion the right thing to do there.

In the example in this ticket, it seems to break because it tries to deduce the block dimensions in a single pass through the cells. In this pass, it only determines the size of column 2 and rows 1 and 2. It might need multiple passes to find all information.

[wjp]

comment:7

Attachment: 4492_doctest_nonsquare0_block.patch.gz

Robert Bradshaw suggested adding an example that explicitly shows zero blocks may be non-square. I added a patch that adds that to the docstring of block_matrix.

[williamstein]

Attachment: trac_4492.patch.gz

apply only this patch (don't apply wjp's)

[wjp]

comment:9

This breaks the following example:

sage: B = Matrix(ZZ,3,2,[1,2,3,4,5,6])
sage: block_matrix([0,1,B,1])

The problem is that it turns the 0 at the top into a 2x2 zero matrix while
it should be a 3x2 zero matrix, but that can only be deduced after processing the ones further on.

[wjp]

comment:10

Oops, sorry for the broken formatting. Clean version:

sage: B = Matrix(ZZ,3,2,[1,2,3,4,5,6])
sage: block_matrix([0,1,B,1]) 

[wjp]

comment:11

I tried to write a patch for this, but ran into some trouble with the last doctest:

        sage: B = matrix(QQ, 2, 3, range(6))
        sage: block_matrix([~A, B, B, ~A], subdivide=False)
        [-5/12   3/8     0     1     2]
        [  1/4  -1/8     3     4     5]
        [    0     1     2 -5/12   3/8]
        [    3     4     5   1/4  -1/8]

In this case there are no real columns as such, and I'm not sure how we should behave if there were an extra row with '1 0' below the '~A B' and 'B ~A' rows. Should that give a 3x3 identity matrix and a 3x2 zero matrix, or a 2x2 identity matrix and a 2x2 identity matrix? Maybe undefined behaviour, or an exception?

My current attempt raises an exception for this doctest that the column widths are inconsistent.

[wjp]

comment:12

For those interested, my current work-in-progress patch is at

http://www.math.leidenuniv.nl/~wpalenst/sage/sage_WIP_block_matrix.patch

[wjp]

comment:13

Further work in progress (replacing the previous patch). This also addresses #2429.

http://www.math.leidenuniv.nl/~wpalenst/sage/block_matrix_2.patch

[wjp]

Author: Willem Jan Palenstijn

[adeines]

comment:15

The following tests failed:

----------------------------------------------------------------------

The following tests failed:

	sage -t  devel/sage/sage/matrix/matrix2.pyx # 2 doctests failed
	sage -t  devel/sage/sage/combinat/designs/incidence_structures.py # 1 doctests failed
	sage -t  devel/sage/sage/crypto/lattice.py # 9 doctests failed
	sage -t  devel/sage/sage/combinat/matrices/hadamard_matrix.py # 1 doctests failed
----------------------------------------------------------------------

[adeines]

Reviewer: Aly Deines

[wjp]

comment:20

The last three, in order:

Apply 4492_block_matrix.patch, trac_4492-block-matrix-reviewer.patch, 4492_typo.patch

[wjp]

comment:21

The reviewer patch looks good, and passes all tests for me too. The doctests and documentation it adds are really clarifying. (I did add a very minor patch on top of it fixing two small typos.)

[adeines]

comment:22

It looks good and all tests pass for me too.

[jdemeyer]

comment:24

This needs to be rebased to sage-4.6.2.alpha1

[jdemeyer]

Work Issues: rebase

[wjp]

rebased to 4.6.2.alpha1

[wjp]

comment:25

Attachment: 4492_block_matrix_rebased.patch.gz

[wjp]

comment:26

Rebased patch attached. (No actual changes, just the context of one of the hunks had changed.)

[rbeezer]

comment:27

Attachment: trac_4492-doctest-number-field.patch.gz

Rebased patches apply fine and Sage builds.

However 3 doctests fail. These can be traced to #10433 that snuck into 4.6.2.alpha1 while this was in-progress. ;-) Specifically one 2x2 block matrix built at line 2366 of sage/rings/number_field/number_field_ideal.py

Latest patch rearranges the block matrix construction to meet new requirements for more explicit lists for the block matrix. I've cc'ed David Loeffler if he wants to check off on just that one change. Passes all tests now in sage/rings. All work here is on 4.6.2.alpha1.

Would somebody like to retest the whole package now?

[loefflerd]

comment:29

(The change to number field code looks fine to me.)

[jdemeyer]

comment:30

I will test the patches.

[wjp]

comment:31

I've also run tests with 4.6.2-alpha2 + this ticket, and all tests passed.

[jdemeyer]

Changed work issues from rebase to none

[jdemeyer]

comment:33

Yep, tests are okay.

[rbeezer]

comment:34

David - thanks for the quick check.

Jeroen, Willem - thanks for the testing.

Sounds like the latest patch has been tested and everybody is OK with all of this, so I'm going to switch this back to positive review.

[jdemeyer]

Merged: sage-4.6.2.alpha3