bugGNU Octave - Bugs: bug #43925, isscalar behaves incorrectly for...

 
 

bug #43925: isscalar behaves incorrectly for some user-defined classes

Submitted by:  Julien Bect <jbect>
Submitted on:  Sat 03 Jan 2015 06:24:46 PM UTC  
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  Kai Torben Ohlhus <siko1056>
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Thu 04 Oct 2018 10:37:39 PM UTC, comment #26:

Finally, I pushed a modified version of the patches supplied to default including some note in the NEWS file:
https://hg.savannah.gnu.org/hgweb/octave/rev/ba937c3dce82

Closing this report.

Kai Torben Ohlhus <siko1056>
Project MemberIn charge of this item.
Sat 21 May 2016 08:27:12 AM UTC, comment #25:

Ok, so is it a regression (between 3.8.2 and 4.0.x) or not?

[thinking....]

-----

First element: in my initial post I wrote:

> In Octave 3.8.2, isscalar(x) returns true if numel(x) is 1.


Actually, it seems that both 3.8.2 and 4.0.x define isscalar(x) as (numel(x) == 1), but in 3.8.2 this is in an m-file, whereas in 4.0.x this in C++.

So, I agree that in term of internal implementation, isscalar seems to have been wrong, in principle, all the time.

-----

Second: the main case for a regression came, IIRC, from a comment by Oliver Heimlich (initial post of bug #44498):

> In GNU Octave 3.8.2 the overriden size had been respected by isscalar, isvector, and isequal.


I don't think I ever checked myself what was going on in 3.8.2.

-----

Thinking about Oliver's comment:

a) The case of isequal different, and has already been dealt with in bugs #44334 and #45118, so let's forget about it.

b) The case of isvector is the subject of bug #44498. In 3.8.x in was an m-file implementation calling size (which is ok) and ndims (which is not ok). So, I would say that the behaviour was ok for two-dimensional arrays and wrong for higher-dimensional arrays. In 4.0.x, issvector is implemented in C++ and used ndims(), so it never respects the oveloaded size. Summary for the case of issvector: for isvector, we have a Matlab-compatibility regression in the case of two-dimensional arrays.

c) The case of isscalar still puzzles me. Given my remark above, I now think It has been wrong all the time, being based on numel. The only explanation that I can think of is that Oliver had numel overloaded in his test class, equal to prod (size (x)).

-----

About some other functions:

d) rows, columns: The behaviour hasn't changed. Overloaded size has never been respected. Note that these functions do not exist in Matlab. Therefore, as far as they are concerned, Matlab-compatibility is not an issue. Only consistency with the other functions is.

e) isrow, iscolumn: Same case as isvector. 3.8.x had an m-file implementation based on size and ndims => Matlab-compatiblity regression for two-dimensional arrays.

f) issquare : 3.8.x also had an m-file implementation based on size and ndims, which thus correctly respected overloaded size functions in the case of two-dimensional arrays. Now it doesn't respect it at all. This is a change of behaviour, but not a Matlab-compatinility issue since the function does not exist in Matlab.

g) ismatrix: Very different case. Was not at all Matlab-compatible in 3.8.x (this has been discussed elsewhere). The 4.0.x version is better in terms of Matlab compatiblity, but ignores the overloaded size.

-----

Summary: I believe that

  • we have several regressions between 3.8.2 and 4.0.x wrt supporting overloaded size: isvector, isrow, iscolumn.
  • fixing these functions but not the others would be a bad idea for two reasons: 1) Even they have not regressed, the other functions are faulty in terms of Matlab-compatibility, and 2) all these size-related functions should behave similarly.

-----

Sorry for the very long post.

Now, the decisions to be made is: do we fix this on stable, on default, or do we fix the regressions only on stable and the rest of it on default ?

I would vote for fixing everything on stable...

Julien Bect <jbect>
Fri 20 May 2016 10:47:38 PM UTC, comment #24:

I can see that this bug is a Matlab compatibility problem that should be fixed, but I'm having a hard time understanding what the regression is.

I've made a simple class example that overrides the size method and I don't see any change at all w.r.t isscalar (incorrectly depends on numel in all versions), so that is not a regression.

I do see differences in isvector and ismatrix, but I don't think either of them were absolutely correct in 3.8 and broken in 4.0, so I wouldn't call that a regression either. They may be broken in both 3.8 and 4.0 in different ways, but that's not really a regression, is it?

I'm not trying to be difficult, just trying to understand to help find the best way to resolve this. Can you show me what I'm missing?

Granted this is a somewhat constructed example showing that isvector was buggy in 3.8 as well. If I change the size to [2,2,3], then isvector correctly returns false in 3.8 and true in 4.0 and newer.

Mike Miller <mtmiller>
Project Administrator
Fri 20 May 2016 04:33:02 PM UTC, comment #23:

Following up on comment #22:

Here is the definition of octave_base_value::rows() in 3.8.2:

It is not overridden in octave_class.

So, if I understand correctly, the (undocumented) behaviour of octave_base_value::rows() for objects has always been to return the size of the underling struct container, even when size is overloaded to return something different.

Therefore, I believe that this should not be changed as part of this regression fix.

Julien Bect <jbect>
Fri 20 May 2016 04:15:31 PM UTC, comment #22:

@mike, bis (comment #20)

About making value.rows() in C++ return the same thing as rows(x) in the interpreter: I agree that it make sense, but is it a regression, or was it alwas like that?

If the old (3.8.x) behaviour was to return the same value, I would say that this is part of the regression, and therefore must also be fixed in stable. I can prepare a modified patch for that.

If the old behaviour was to return different values, I would say that we should only fix the regression on stable (using the proposed patch) and then discuss this design issue somewhere else (same as comment #21).

Julien Bect <jbect>
Fri 20 May 2016 04:00:30 PM UTC, comment #21:

@Mike (comment #20)

About adding the "const" attribute to size: assuming it is possible, I don't believe such a change would be made on stable, would it?

My patch applies to stable, since it fixes a regression in the 4.0.x series, and I hoped it could be applied before 4.0.3 gets released.

Perhaps could we keep the two issues separate:

  • first fix the regression on stable and close this bug report,
  • second open a separate bug report (or ticket on the patch tracker) to discuss the const-ness of size and perhaps change it?
Julien Bect <jbect>
Thu 19 May 2016 07:13:40 PM UTC, comment #20:

I think I understand what's going on here. At least you seem to have done thorough investigation into the differences between ndims, numel, and size.

I also wonder why size is not const, I suspect it's only because octave_class::size calls octave_class::in_class_method, which calls octave_class::find_parent_class which is non-const. We can easily check for other overrides of octave_base_value::size (at least in Forge repositories) to see if there are any non-const usages out there.

I also wonder if we shouldn't fix octave_class::columns and octave_class::rows along with this change, so that at least stays consistent. I would find it confusing if rows(x) returns one thing at the interpreter level, but value.rows() returns something completely different in C++.

Mike Miller <mtmiller>
Project Administrator
Mon 16 May 2016 08:32:16 AM UTC, comment #19:

Here is a slightly improved version of my patch.

I believe it is now ready to be applied on stable (since it fixes a regression).

(file #37176)

Julien Bect <jbect>
Wed 17 Feb 2016 07:44:53 AM UTC, comment #18:

About Fndims, see http://lists.gnu.org/archive/html/octave-maintainers/2016-02/msg00075.html

Unless something new comes up, I won't try to changes Fndims's behaviour in this patch.

Julien Bect <jbect>
Fri 12 Feb 2016 01:23:45 PM UTC, comment #17:

See the attached patch for work in progress.

It now deals with more functions than we originally thought (issquare, isrow, iscolumn, rows, columns...).

I am still not sure of the right course of action concerning Fndims (see below: Do we assume that it is a bug in Matlab and fix it ?).

Some more work is also required on classes.tst (add new unit tests, check for Matlab compatibility).

(file #36323)

Julien Bect <jbect>
Fri 12 Feb 2016 10:22:45 AM UTC, comment #16:

By the way, I understand now why tests such as (sz(0) >= 0) are useful. The reason is that octave_class doesn't fully check that the result of size, when it is overloaded, is a valid size vector. So, sz(0) could be negative, Nan or infinite...

Julien Bect <jbect>
Fri 12 Feb 2016 10:20:11 AM UTC, comment #15:

Well, I leave it to you to discuss it on the mailing list if you like.

As far as this patch is concerned, we have to make a copy to avoid the problem (size() might become const one day, but certainly not in a 4.0.x release).

Julien Bect <jbect>
Fri 12 Feb 2016 09:15:24 AM UTC, comment #14:

I also found it odd that size() isn't const.

I didn't raise it because it is difficult to change. There is probably code out there that overloads the non-const size(). If we make size() const, that code will fail in complicated ways. That is even true if we have both a const and non-const size(), and we use the const one.

However, that is just my opinion. I haven't seen it discussed on the mailing list, but it would be great to hear what others think.

Lachlan Andrew <lachlan>
Fri 12 Feb 2016 08:28:16 AM UTC, comment #13:

Hello Lachlan,

I see that we have to copy args(0) because size() isn't const.

I find it weird that size() isn't const but numel, ndims, rows, columns... all are!

Is there a rationale for this? Has this been discussed on the mailing list?

@++
Julien

Julien Bect <jbect>
Thu 11 Feb 2016 12:49:48 PM UTC, comment #12:

I have a doubt about ndims()... It would seem natural to me that ndims() should also respect the user-defined size.

But in Matlab (R2012a) this is confusing:

  • The documentation says that ndims() should return length(size(x)).
  • But ndims does not respect that is the size function is overloaded...

I have to check whether this has been fixed in more recent Matlab versions.

Julien Bect <jbect>
Thu 11 Feb 2016 12:40:33 PM UTC, comment #11:

I will prepare a new patch, so until I'm done, yes, you can leave it to me.

Julien Bect <jbect>
Thu 11 Feb 2016 12:35:12 PM UTC, comment #10:

Thanks. I was just about to say that the sz(0) >= 0 was introduced in changeset 00e31f316a3a, with the change

I forgot to update the unit tests. That is a habit I should get into with all patches, I suppose. Is there anything else I can help with on this, or should I leave it to you?

Lachlan Andrew <lachlan>
Thu 11 Feb 2016 12:30:17 PM UTC, comment #9:

Ooops, there are so many typos in my previous message that if find it hard to read... I meant:

For matlab compatibility, ismatrix must return true for sizes such as 0x1 or 1x0. The rule is that ismatrix returns true if the size is [m n] for non-negative integers m and n.

I have just tested without the sz>=0 tests, and all the unit tests still pass. I will prepare a modified patch later.

In the meantime, I am currently which xtest statements in test/classes/classes.tst can now be converted to test.

I have also found out that columns() and rows() suffer from exactly the same problem. I will extend the patch to deal with them as well.

Julien Bect <jbect>
Thu 11 Feb 2016 12:17:47 PM UTC, comment #8:

For matlab compatibility, ismatrix must return true if for sizes such 0x1 or 1x0. The rule is that ismatrix returns true is the size is [m n] for non-negative integers m and n.

Julien Bect <jbect>
Thu 11 Feb 2016 11:02:24 AM UTC, comment #7:

Good catch regarding sz(.) >= 0. I just copied what was there...

My guess is that they should be sz(.) > 0, since something isn't really a matrix if it is empty. Or perhaps they used to be >0, and someone "fixed" them to be >=0 to explicitly allow empty matrices. I'll take a look for that.

Lachlan Andrew <lachlan>
Wed 10 Feb 2016 04:09:50 PM UTC, comment #6:

I have rebased the patch to stable (since it fixes a regression, I believe it should be included in 4.0.1) and added a similar fix for issquare.

I have tested all four functions (isscalar, isvector, ismatrix, issquare) and everything looks fine to me.

The tests in ismatrix (sz(0) >= 0 && sz(1) >= 0) should be removed if there is no reason for keeping them. Otherwise, I think that a comment should be added to explain why they are necessary.

(file #36300)

Julien Bect <jbect>
Wed 10 Feb 2016 01:06:14 PM UTC, comment #5:

What is the point of testing for sz(0) >= 0 && sz(1) >= 0 in ismatrix ?

Julien Bect <jbect>
Wed 10 Feb 2016 12:47:22 PM UTC, comment #4:

I suggest retagging this bug as regression (same as bug #44498) since the problem didn't exist in Octave 3.8.2.

Julien Bect <jbect>
Sat 30 Jan 2016 02:41:41 AM UTC, comment #3:

The attached patch fixes this.

It also fixes bug #44498 (isvector), and "ismatrix" which seems not to have been reported.

Could someone please test it, and (someone else?) apply it? Thanks.

(file #36205)

Lachlan Andrew <lachlan>
Sun 04 Jan 2015 03:16:52 PM UTC, comment #2:

I have updated the suite of unit test for user-defined classes to include additional tests that fail on this isscalar problem.

Please consider pulling from my octave repo:

https://bitbucket.org/jbect/octave

the four most recent changesets (b1de63b, 778c39d,
1ee4035 and f0a5dd0).

Julien Bect <jbect>
Sat 03 Jan 2015 06:47:30 PM UTC, comment #1:

Please also see bug #42422 for a similar problem with ismatrix.

Andreas Weber <andy1978>
Project Member
Sat 03 Jan 2015 06:24:46 PM UTC, original submission:

In Octave 3.8.2, isscalar(x) returns true if numel(x) is 1.

In Matlab, isscalar(x) returns true if size(x) is [1 1].

http://www.mathworks.com/help/matlab/ref/isscalar.html

These two conditions are not always equivalent.

For instance, in the case of a user-defined class that overloads size, it is possible to have size (x) equal to [6 4] but numel (x) equal to 1. (And overloading numel too is not always an option, since the result of numel matters for subsref.)

isscalar has been rewritten in C++ on the default branch, but the behaviour is still the same.

Julien Bect <jbect>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #36323:  RespectUserDefinedSize.patch added by jbect (9KiB - text/x-patch)
file #36300:  bug_43925_stable.patch added by jbect (2KiB - text/x-patch)
file #36205:  bug_43925.cset added by lachlan (2KiB - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by andy1978 (Posted a comment)
  • -email is unavailable- added by jbect (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-19 mtmiller Carbon-CopyRemoved 80942 => -
    2018-10-04 siko1056 StatusPatch Submitted => Fixed
        Assigned toNone => siko1056
        Open/ClosedOpen => Closed
    2018-10-02 siko1056 Dependencies- => bugs #51308 is dependent
    2017-02-23 siko1056 Dependencies- => bugs #44498 is dependent
    2017-01-03 lachlan Assigned tolachlan => None
    2016-05-16 jbect Attached File- => Added RespectUserDefinedSize.v2.patch, #37176
    2016-02-12 jbect Attached File- => Added RespectUserDefinedSize.patch, #36323
    2016-02-11 lachlan StatusReady For Test => Patch Submitted
    2016-02-10 jbect Attached File- => Added bug_43925_stable.patch, #36300
    2016-02-08 lachlan StatusNone => Ready For Test
    2016-02-08 lachlan Assigned toNone => lachlan
        Release3.8.2 => dev
        Operating SystemGNU/Linux => Any
    2016-01-30 lachlan Attached File- => Added bug_43925.cset, #36205

    Back to the top


    Powered by Savane 3.4