Opened 12 years ago
Closed 10 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: |
Description (last modified by )
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:
Attachments (7)
Change History (42)
comment:1 Changed 12 years ago by
- Milestone set to sage-3.2.1
comment:2 Changed 12 years ago by
- Cc craigcitro added
comment:3 Changed 12 years ago by
- Cc jason added
- Component changed from algebra to linear algebra
- Owner changed from tbd to was
comment:4 Changed 11 years ago by
- Report Upstream set to N/A
comment:5 Changed 11 years ago by
- Status changed from new to needs_review
comment:6 Changed 11 years ago by
- 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 11 years ago by
comment:7 Changed 11 years ago by
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 11 years ago by
- Status changed from needs_work to needs_review
comment:9 Changed 11 years ago by
- 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 11 years ago by
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 11 years ago by
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 11 years ago by
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 10 years ago by
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 10 years ago by
- Status changed from needs_work to needs_review
comment:15 Changed 10 years ago by
- 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 ----------------------------------------------------------------------
comment:16 Changed 10 years ago by
- Status changed from needs_work to needs_review
Oops, sorry about that. New patch up that fixes those.
Changed 10 years ago by
comment:17 Changed 10 years ago by
- 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 10 years ago by
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 10 years ago by
comment:19 Changed 10 years ago by
How should the patches be applied? If I try either of the last two by themselves, they don't apply.
comment:20 Changed 10 years ago by
The last three, in order:
Apply 4492_block_matrix.patch, trac_4492-block-matrix-reviewer.patch, 4492_typo.patch
comment:21 Changed 10 years ago by
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 10 years ago by
- Status changed from needs_review to positive_review
It looks good and all tests pass for me too.
comment:23 Changed 10 years ago by
- Description modified (diff)
comment:24 Changed 10 years ago by
- Status changed from positive_review to needs_work
- Work issues set to rebase
This needs to be rebased to sage-4.6.2.alpha1
comment:25 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:26 Changed 10 years ago by
Rebased patch attached. (No actual changes, just the context of one of the hunks had changed.)
Changed 10 years ago by
comment:27 Changed 10 years ago by
- 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 10 years ago by
- Description modified (diff)
comment:29 Changed 10 years ago by
(The change to number field code looks fine to me.)
comment:30 Changed 10 years ago by
I will test the patches.
comment:31 Changed 10 years ago by
I've also run tests with 4.6.2-alpha2 + this ticket, and all tests passed.
comment:32 Changed 10 years ago by
- Work issues rebase deleted
comment:33 Changed 10 years ago by
Yep, tests are okay.
comment:34 Changed 10 years ago by
- 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 10 years ago by
- Merged in set to sage-4.6.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
So I figured out what is going wrong here. Patch to come shortly.