Opened 12 years ago

Closed 9 years ago

#6569 closed defect (fixed)

sparse integer matrix doesn't raise an error on non-integer index

Reported by: jason Owned by: was
Priority: major Milestone: sage-5.0
Component: linear algebra Keywords:
Cc: rbeezer, mjo Merged in: sage-5.0.beta2
Authors: Michael Orlitzky Reviewers: William Stein
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

{{{
sage: a=matrix(4,range(1,17),sparse=True)
sage: a[[2.3],[2.1]]                     
[0]
}}}

as compared to

{{{
sage: a=matrix(4,range(1,17))            
sage: a[[2.3],[2.1]]         
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/grout/.sage/temp/tiny/8143/_home_grout__sage_init_sage_0.py in <module>()

/home/grout/sage/local/lib/python2.6/site-packages/sage/matrix/matrix0.so in sage.matrix.matrix0.Matrix.__getitem__ (sage/matrix/matrix0.c:4999)()

/home/grout/sage/local/lib/python2.6/site-packages/sage/matrix/matrix1.so in sage.matrix.matrix1.Matrix.matrix_from_rows_and_columns (sage/matrix/matrix1.c:8613)()

TypeError: 'sage.rings.real_mpfr.RealLiteral' object cannot be interpreted as an index
}}}

Attachments (1)

sage-trac_6569.patch (3.6 KB) - added by mjo 9 years ago.
Catch invalid indices in Matrix.getitem and add doctests.

Download all attachments as: .zip

Change History (5)

Changed 9 years ago by mjo

Catch invalid indices in Matrix.getitem and add doctests.

comment:1 Changed 9 years ago by mjo

  • Authors set to Michael Orlitzky
  • Cc mjo added
  • Report Upstream set to N/A
  • Status changed from new to needs_review

Some invalid indices weren't being caught in the general matrix[foo] accessor; the fact that non-sparse matrices failed with an error was an implementation detail.

I think I've fixed them all, and doctested that sparse/dense behave the same way now.

comment:2 Changed 9 years ago by was

  • Status changed from needs_review to positive_review
  1. I just looked at this and I'm shocked that ind is of type int. This will work fine on 32-bit, but will be totally broken/buggy in subtle and surprising ways on 64-bit machines. The type of ind should be Py_ssize_t, just like the type of i.

I realize that your patch does not introduce ind. It was introduced in #4973...

Nobody will notice this now, because there is some sort of massive weird inefficiency in the creation of sparse matrices (maybe the parent space has its basis constructed or something idiotic like that), which makes it impossible to make a large sparse matrix, even though this should be trivial, instant, fast, and take no memory:

sage: time a = matrix(QQ, 2^25, sparse=True)
Time: CPU 5.88 s, Wall: 8.46 s
sage: get_memory_usage()   # what ?
3908.29296875

I've made the ind and memory issue trac #12348.

  1. That said, everything in this particular patch looks great to me. Positive review!

I hope you can address #12348 though next. At least the trivial change of ind to Py_ssize_t.

comment:3 Changed 9 years ago by jdemeyer

  • Reviewers set to William Stein

comment:4 Changed 9 years ago by jdemeyer

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