Opened 13 years ago

Closed 11 years ago

#4492 closed defect (fixed)

block_matrix reacts inconsistently with 0

Reported by: jbmohler Owned by: was
Priority: major Milestone: sage-4.6.2
Component: linear algebra Keywords:
Cc: craigcitro, jason, davidloeffler Merged in: sage-4.6.2.alpha3
Authors: Willem Jan Palenstijn Reviewers: Aly Deines, Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by wjp)

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. 4492_block_matrix_rebased.patch
  2. trac_4492-block-matrix-reviewer.patch
  3. 4492_typo.patch
  4. trac_4492-doctest-number-field.patch

Attachments (7)

4492_doctest_nonsquare0_block.patch (914 bytes) - added by wjp 12 years ago.
trac_4492.patch (1.9 KB) - added by was 12 years ago.
apply *only* this patch (don't apply wjp's)
4492_block_matrix.patch (26.4 KB) - added by wjp 11 years ago.
block_matrix rewrite. Replaces all previous patches.
trac_4492-block-matrix-reviewer.patch (13.0 KB) - added by rbeezer 11 years ago.
4492_typo.patch (1.3 KB) - added by wjp 11 years ago.
4492_block_matrix_rebased.patch (26.4 KB) - added by wjp 11 years ago.
rebased to 4.6.2.alpha1
trac_4492-doctest-number-field.patch (898 bytes) - added by rbeezer 11 years ago.

Download all attachments as: .zip

Change History (42)

comment:1 Changed 13 years ago by jbmohler

  • Milestone set to sage-3.2.1

comment:2 Changed 13 years ago by craigcitro

  • Cc craigcitro added

comment:3 Changed 13 years ago by jason

  • Cc jason added
  • Component changed from algebra to linear algebra
  • Owner changed from tbd to was

comment:4 Changed 12 years ago by sdietzel

  • Report Upstream set to N/A

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

comment:5 Changed 12 years ago by sdietzel

  • Status changed from new to needs_review

comment:6 Changed 12 years ago by wjp

  • Status changed from needs_review to needs_work

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.

Changed 12 years ago by wjp

comment:7 Changed 12 years ago by wjp

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.

comment:8 Changed 12 years ago by was

  • Status changed from needs_work to needs_review

Changed 12 years ago by was

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

comment:9 Changed 12 years ago by wjp

  • Status changed from needs_review to needs_work

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.

comment:10 Changed 12 years ago by wjp

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

comment:11 Changed 12 years ago by wjp

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.

comment:12 Changed 12 years ago by wjp

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

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

comment:13 Changed 11 years ago by wjp

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

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

comment:14 Changed 11 years ago by wjp

  • Authors set to Willem Jan Palenstijn
  • Status changed from needs_work to needs_review

comment:15 Changed 11 years ago by aly.deines

  • Reviewers set to Aly Deines
  • Status changed from needs_review to needs_work

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

Changed 11 years ago by wjp

block_matrix rewrite. Replaces all previous patches.

comment:16 Changed 11 years ago by wjp

  • Status changed from needs_work to needs_review

Oops, sorry about that. New patch up that fixes those.

Changed 11 years ago by rbeezer

comment:17 Changed 11 years ago by rbeezer

  • Reviewers changed from Aly Deines to Aly Deines, Rob Beezer

Looks real nice. I've added some edits in a reviewer patch.

I'll run full tests overnight (with reviewer patch), and then will be ready to give this a positive review (subject to acceptance of my reviewer edits).

comment:18 Changed 11 years ago by rbeezer

All tests in sage/devel pass with reviewer patch applied. So I am all clear on a positive review. Once the reviewer patch gets a review, this can be flipped to "positive review".

Nice contribution - this will be very useful.

Rob

Changed 11 years ago by wjp

comment:19 Changed 11 years ago by aly.deines

How should the patches be applied? If I try either of the last two by themselves, they don't apply.

comment:20 Changed 11 years ago by wjp

The last three, in order:

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

comment:21 Changed 11 years ago by wjp

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

comment:22 Changed 11 years ago by aly.deines

  • Status changed from needs_review to positive_review

It looks good and all tests pass for me too.

comment:23 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:24 Changed 11 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to rebase

This needs to be rebased to sage-4.6.2.alpha1

Changed 11 years ago by wjp

rebased to 4.6.2.alpha1

comment:25 Changed 11 years ago by wjp

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:26 Changed 11 years ago by wjp

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

Changed 11 years ago by rbeezer

comment:27 Changed 11 years ago by rbeezer

  • Cc davidloeffler added

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?

comment:28 Changed 11 years ago by wjp

  • Description modified (diff)

comment:29 Changed 11 years ago by davidloeffler

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

comment:30 Changed 11 years ago by jdemeyer

I will test the patches.

comment:31 Changed 11 years ago by wjp

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

comment:32 Changed 11 years ago by jdemeyer

  • Work issues rebase deleted

comment:33 Changed 11 years ago by jdemeyer

Yep, tests are okay.

comment:34 Changed 11 years ago by rbeezer

  • Status changed from needs_review to positive_review

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.

comment:35 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.