Opened 7 years ago

Closed 7 years ago

#13012 closed defect (fixed)

MatrixSpace is too eager to construct zero matrices

Reported by: novoselt Owned by: jason, was
Priority: major Milestone: sage-5.1
Component: linear algebra Keywords: sd40.5
Cc: vbraun, kcrisman, rbeezer Merged in: sage-5.1.beta5
Authors: Andrey Novoseltsev Reviewers: William Stein
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

This is a follow up to #10793 where it was prohibited to silently change dimensions of a matrix preserving the number of entries (e.g. 3x2 to 2x3). Here is the problem with zeros:

sage: m = zero_matrix(2, 3)
sage: m
[0 0 0]
[0 0 0]
sage: M = MatrixSpace(ZZ, 3, 5)
sage: M.zero()
[0 0 0 0 0]
[0 0 0 0 0]
[0 0 0 0 0]
sage: M(m)
[0 0 0 0 0]
[0 0 0 0 0]
[0 0 0 0 0]

The last line should have raised an exception. I'm writing a patch.

Attachments (1)

trac_13012_bug_with_zero_matrices.patch (19.2 KB) - added by novoselt 7 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by novoselt

  • Authors set to Andrey Novoseltsev
  • Status changed from new to needs_review

comment:2 Changed 7 years ago by novoselt

  • Status changed from needs_review to needs_work
  • Work issues set to doctests

comment:3 Changed 7 years ago by novoselt

Not yet finished, but I am doing the following:

  • concentrate matrix constructor code in MatrixSpace.matrix (currently a lot is done in its __call__);
  • quit automatic determination of "data as columns" - this is dangerous since if the code relying on it ever hits a square matrix, it will give a hard to find mistake;
  • drop support for constructing matrices from arbitrary combination of matrices/vectors/scalars introduced at #10628 (and released in Sage-5.0). This was not the goal of that ticket and I think it is a dangerous functionality since it easily lets through wrong code on "corner cases": those who deliberately want to augment or stack matrices should use corresponding methods.

If you object to anything on this list, please talk to me! I think that correctness of this approach is indirectly confirmed by the fact that only tests in matrix_space that demonstrate removed functionality fail in the matrix directory (a few tests fail because of a more informative error message).

Changed 7 years ago by novoselt

comment:4 Changed 7 years ago by novoselt

  • Status changed from needs_work to needs_review

Summary of what was done:

  • MatrixSpace.__call__ just calls MatrixSpace.matrix, so that the logic is the same for both ways.
  • row keyword is deprecated and no guessing for row=False is done - this was used only in matrix_space for demonstration, so it is unlikely that users use it either.
  • Matrices are now constructed either from a list of entries or from a list of rows, there are special methods for stacking/augmenting matrices.

Two files had to be adjusted:

  • linear_code had code which was very strange, I have rewritten it.
  • matrix_morphism exploited undocumented functionality - I have removed it and fixed the test.

comment:5 Changed 7 years ago by was

  • Status changed from needs_review to positive_review

LGTM

comment:6 Changed 7 years ago by jdemeyer

Please fill in your real name in the Author / Reviewer fields.

comment:7 Changed 7 years ago by novoselt

  • Reviewers set to William Stein

comment:8 Changed 7 years ago by jdemeyer

  • Work issues doctests deleted

comment:9 Changed 7 years ago by jdemeyer

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