Re: purpose of 10486:4e64fbbd5c58

[Jaroslav Hajek]

On Tue, Apr 6, 2010 at 6:01 PM, John W. Eaton <address@hidden> wrote:
> On  6-Apr-2010, Jaroslav Hajek wrote:
>
> | maybe we should discuss this change. What's the point of this?
> | OK, now it's Matlab-compatible, but I think Octave's behavior was
> | better. I really hate it that x([a:b]) now behaves differently from
> | x(a:b).
>
> I agree that it would be better to require indices to be integers.
> The change was made after I received a bug report from someone who is
> paying me for support and who noticed the incompatibility with
> Matlab.

OK, that's a strong point. Still, I'd try to explain to him that this
is a particularly nasty feature to rely on.

> It seemed to me that it would be a relatively safe change
> since it only relaxed the rules for indexing slightly.  It also seemed
> best to me to make the change at a low level so that any code that
> created an idx_vector object from a Range would work the same way.

That's exactly the bad idea. The index_vector query is used by a
number of other methods, for instance, sub2ind and ind2sub, none of
which do allow non-integer ranges to be passed in. Actually this
change has likely made Octave less Matlab-compatible than it was.

> I didn't discuss the change because it seemed pretty straightforward and
> I didn't see it causing any trouble with existing code.
>
> | IMHO this is simply a defect in Matlab that persists for compatibility
> | and will likely be removed in the future.
>
> Since the warning message from Matlab says that integer indexing is
> required, but then it allows non-integer indexing in this particular
> case, it could also be that at some point a change was made to make
> non-integer indexing with ranges an error, then someone complained
> that the change broke a lot of their code (I know, why would they be
> doing this in the first place, but they are, so they don't want to
> be forced to modify all their code) then the "fix" was to just make
> the error a warning.  But that's of course just a guess.
>

Perhaps. Let me guess as well. It doesn't seem that MathWorks is
determined to keep backward compatibility quirks forever. The most
recent version of Matlab did, according to release notes, break far
more important things than this. For instance, scalar indexing to
sparse matrices now returns a sparse matrix.
There's a good reason for this, actually, because it creates a nasty
special case to watch for, while getting the scalar result is as easy
as doing "full (s(i,j))".
Note that Octave has been doing this right for ages now. I don't know
whether nobody else noticed this difference - I did, but I decided not
to fix it. Had we decided otherwise, we would now probably be changing
it back.
I'm saying this to point out that ultimate Matlab compatibility is a
fragile thing to follow. The Matlab people make wrong decisions, but
they also revise their decisions. And this was a documented feature.

> | Besides breaking isindex(),
>
> How does it break isindex?
>

With your change, isindex (1.1:4) was true (and showed a warning).
That's bad. Note that it is intentional that isindex uses a trial
index_vector query, to possibly cache the check.


> | this breaks the general consistency; an
> | invalid index is accepted in some contexts but rejected in others.
>
> Will you please give an example of the inconsistent behavior?  If I
> had recognized an inconsistency (other than the different treatment of
> matrices and ranges) I would have probably thought about whether there
> was a better way to make the change.

For instance, sub2ind and ind2sub were affected - they accepted the
invalid ranges (while in Matlab they don't). accumarray was probably
affected, too. sparse, too, with the recent changes.


>
> | Since it is nearly impossible to distinguish a range from a matrix in
> | an m-function, this will be difficult to honor in user-defined
> | classes, breaking consistency even more.
>
> I don't understand.  You seem to be thinking about examples that I am
> unable to guess, so it would help if you could show the examples where
> this change will cause trouble.

It's simple, isn't it? Matlab allows this stupidity *only* if it's a
range, not if it's converted to a matrix first.
Even storing the range to a variable makes a difference. Perhaps
Matlab can't store ranges to variables at all?
So, if you write a user class that needs to overload subsref, and you
want it to be consistent with the built-in indexing, you need to
distinguish between a range and a range that has been converted to
matrix. That is difficult and it essentially can't be done without
typeinfo.

>
> | Are there any positives that I'm missing?
>
> The positive is that it makes Octave compatible with Matlab in this
> one small way, so that people who happen to use this feature (and yes,
> I agree that it is not a good feature) won't trip over the difference
> when they try to use their code in Octave, and then subsequently
> conclude that Octave sucks.
>
> jwe
>

Maybe you could try to explain to the person why it's bad in general
and why it causes trouble for Octave, and point out that it is not
documented, so it can easily stop working in the future.
You can't in general prevent idiots from concluding that Octave sucks.
If anyone makes such conclusion out of such misfeatures, he's probably
lost for the Octave community anyway. Octave did intentionally avoid
copying some Matlab defects in the past, this seems to be one of them.
On the contrary, if Octave sacrifices code clarity, performance, and
better functionality just to copy certain Matlab quirks, you can
convince existing users and developers that Octave sucks.

-- 
RNDr. Jaroslav Hajek, PhD
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz