Re: [Octave-bug-tracker] [bug #40341] Logical indexing into sparse matrices causes OOM

[Jordi Gutiérrez Hermoso]

On Tue, 2013-10-22 at 09:33 +0200, David Bateman wrote:
> Here are some principals for the sparse implementation
> 
> 1) The row and column indices of the sparse matrix should be of the
> type octave_idx_type, limited to [0 intmax()] and not artificially
> limited to something smaller

Agreed.

> 2) numel() should have a return type of octave_idx_type and defined
> as the total number of zero and non-zero elements if the matrix and
> so limited to [0 intmax()]

This one I disagree with. It obviously has to be true for full
matrices because we need to be able to linearly index as much as
numel() elements, but it's an unnatural restriction for sparse
matrices. Instead, we should patch numel() to return double and handle
the locations where this fails by letting the compiler handle this (as
an intermediate step, we can pick another type so that there are no
implicit cast from double to octave_idx_type and let the compiler find
these locations for us).

The problem is that you wrote sparse with some assumptions, and when
your assumptions are no longer valid, you're blaming the calling code
for not following the assumptions of your internal implementation
details. In order to fix this, you're making your assumptions leak out
of your implementation into the calling code, breaking encapsulation,
playing whack-a-mole with each location that violates your
assumptions, and increasing the maintenance burden on everyone,
including yourself, and introducing some new bugs as you go along (and
fixing it with j++, seriously?).

What are you going to do when someone actually does call numel() for a
sparse matrix from the Octave interpreter? You can't leak your failed
encapsulation all the way to user-submitted code. I'm waiting for
David Spies to run into that bug.

By the way, it's been a while since you've been actively involved in
Octave development, and I haven't contributed much myself either
lately in actual code, but nowadays when we fix a bug, we include a
test for it. I'll go later and add tests to each mole you've whacked,
if you don't beat me to it.
 
> The corollary is that numel()

If I disagree with your assumptions, your corollaries are not valid to
me either.

- Jordi G. H.