Opened 8 years ago

Last modified 6 years ago

#13564 needs_info enhancement

Improve diagonal from matrix/matrix2.pyx

Reported by: r.gaia.cs Owned by: jason, was
Priority: major Milestone: sage-6.4
Component: linear algebra Keywords: matrix, diagonal
Cc: Merged in:
Authors: Raniere Silva Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by ppurka)

The method diagonal could have a parameter so that it can return any diagonal parallel to the main diagonal.


Apply only trac_13564-diagonal.patch to devel/sage.

Attachments (2)

diagonal.patch (3.4 KB) - added by r.gaia.cs 8 years ago.
trac_13564-diagonal.patch (3.4 KB) - added by r.gaia.cs 8 years ago.
Make correction sugested by ppurka.

Download all attachments as: .zip

Change History (13)

Changed 8 years ago by r.gaia.cs

comment:1 Changed 8 years ago by mhansen

  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 8 years ago by ppurka

Hello, this comment is with respect to the statement "- Unknown: No author specified in the file from 2012-10-03" in your patch. The authors are mentioned in the beginning of the file.

As such, I think typically no authors are added at the top unless some major change is effected. The author names and history is preserved in mercurial anyway. You can run sage -hg blame filename to see which revision modified which line of the file filename. The revision number can then be used to pinpoint the author.

Changed 8 years ago by r.gaia.cs

Make correction sugested by ppurka.

comment:3 Changed 8 years ago by ppurka

  • Description modified (diff)

comment:4 Changed 8 years ago by ppurka

Have to force the patchbot :( Patchbot apply trac_13564-diagonal.patch

comment:5 Changed 8 years ago by ppurka

Some comments:

  1. Should we be raising an error or simply returning an empty list if the d for the d-th diagonal is provided outside the range of the number of rows or columns? For instance, the empty matrix is returning an empty diagonal. Also, Mathematica and Matlab silently return empty matrices. Octave raises an error. Numpy gives an empty matrix until nrows() or ncols() and then raises an error.
  2. If it seems more proper to raise an error, the error string could be written like the statements below. Note that d is always less than and never equal to nrows() or ncols().
    The value `d` should be less than the number of columns in the matrix.
    The value `-d` should be less than the number of rows of the matrix.
    

EDIT: The numpy error seems to stem from its refusal to create arrays which have one dimension negative. So, 0x1 array is ok, but not -1x1. The error is probably not from the diagonal code itself.

Last edited 8 years ago by ppurka (previous) (diff)

comment:6 follow-up: Changed 8 years ago by knsam

  • Cc sage-devel added
  • Status changed from needs_review to needs_info

I am changing this to "needs_info" for now as it seems to me that, there is still some design decisions to make. I am adding cc: sage-devel so that this could be discussed.

comment:7 in reply to: ↑ 6 Changed 8 years ago by ppurka

  • Authors changed from r.gaia.cs to Raniere Silva
  • Cc sage-devel removed

Replying to knsam:

I am changing this to "needs_info" for now as it seems to me that, there is still some design decisions to make. I am adding cc: sage-devel so that this could be discussed.

There is no user called "sage-devel". If you want to "cc sage-devel" then you need to put a post to the sage-devel mailing list. I was waiting for a reply from the author for clarification on the current implementation - thanks for setting this to needs_info. However, the author seems inactive since last November.

Last edited 8 years ago by ppurka (previous) (diff)

comment:8 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:10 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:11 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4
Note: See TracTickets for help on using tickets.