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

 
 

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

Submitter:  Julien Bect <jbect>
Submitted:  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:  siko1056
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( 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 Member
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?

>> x = myobj (rand (1, 2, 3));
>> size (x)
ans =

   1   2   3

>> isscalar (x)
ans =  1  ## True in Octave 3.6, 3.8, 4.0, and default
>> isvector (x)
ans =  1  ## True in Octave 3.6, 3.8, 4.0, and default
>> ismatrix (x)
ans = 0   ## False in Octave 3.6 and 3.8
ans =  1  ## True in Octave 4.0 and default

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:

octave_idx_type rows (void) const
{
  const dim_vector dv = dims ();
  return dv(0);
}

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

-      octave_value arg = args(0);
-
-      retval = arg.is_matrix_type ()
-               || arg.is_scalar_type ()
-               || arg.is_range ();
+      dim_vector sz = args(0).dims ();
+      retval = (sz.length () == 2) && (sz(0) >= 0) && (sz(1) >= 0);

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)

 

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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    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.9