bugGNU Octave - Bugs: bug #51308, ndims, rows, columns incorrect for...

 
 

bug #51308: ndims, rows, columns incorrect for classes that only overload size

Submitter:  John W. Eaton <jwe>
Submitted:  Sat 24 Jun 2017 11:57:04 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  siko1056
Originator Name:  jwe Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 24 Jan 2019 10:51:33 PM UTC, comment #7: 

It was left open as a reminder to tag the asserts of the cset in comment #5 with the bug number.  Back then I could not find a proper documentation/best practice for it and asked on the mailing-list https://lists.gnu.org/archive/html/octave-maintainers/2018-10/msg00006.html without reply yet.

The issue of this report is fixed and I think it is documented enough by the comment to use "size" for dimension determination.

Kai Torben Ohlhus <siko1056>
Group Member
Thu 24 Jan 2019 06:56:23 PM UTC, comment #6: 

Kai - is there still something to be done to add tests for this change on the stable branch or can this be closed?

Mike Miller <mtmiller>
Group Member
Thu 04 Oct 2018 10:29:03 PM UTC, comment #5: 

Finally, I pushed it to default including some note in the NEWS file:
https://hg.savannah.gnu.org/hgweb/octave/rev/ba937c3dce82

The last thing to care about are the XTests.  Regarding this I ask a question on the mailing-list.  Thus leave it open for now.

Kai Torben Ohlhus <siko1056>
Group Member
Wed 03 Oct 2018 02:34:15 PM UTC, comment #4: 

Although still (remotely) possible, I don't plan to make another stable release before Octave 5.

In any case, this does seem like a change that should be made on default.

John W. Eaton <jwe>
Group administrator
Wed 03 Oct 2018 10:30:32 AM UTC, comment #3: 

Mike, I see your concerns.  My arguments to speed up the process (despite personal interest ;-) ) are best explained by tracing down the change for the "isvector" function [1], that effectively nothing changes:

Current:

dim_vector sz = args(0).dims ();
return ovl (sz.ndims () == 2 && (sz(0) == 1 || sz(1) == 1));


Proposed:

Matrix sz = octave_value (args(0)).size ();
return ovl (sz.numel () == 2 && (sz(0) == 1 || sz(1) == 1));


Thus "dims" is only replaced by "size", which, in the absence of undiscovered bugs, returns the exact same value (in a different data container Matrix vs. dim_vector).  Further "dims" is constant, while "size" is not (due to undocumented but comprehensible facts [2]) and makes a octave_value-wrapping necessary.  On the other hand, "size" is overloadable.

- The aforementioned functions might be marginally slower, due to the wrapping (no figures to prove or disprove this).  But compiler optimization should cope with the situation for builtin types.

+ The change affects only the pure interpreter functions "isscalar", "isvector", ... without liboctave changes, just by calling the "same" function.

+ For builtin datatypes nothing can change (the code is compiled), we only improve the situation for user-defined classes.

+ The change comes in a few months, rather than next year (some users, among them Forge developers, are waiting since 2015 for this change).

If you or others still disagree, I will just push it to default and will do nothing before getting feedback.


[1] https://hg.savannah.gnu.org/hgweb/octave/file/868830474750/libinterp/corefcn/data.cc#l3651
[2] It took me a while to understand this.  For builtin datatypes, this is pure nonsense.  Why should polling the size of an object change it's contents?  In 2006, when jwe introduced this function [3], it was constant.  Later in 2009 Jaroslav Hajek removed const, to enable user-defined classes [4].  He needed to call the interpreter "feval" [5] for evaluating the user defined "size" method.  And when calling the interpreter, the control over the object is given away, thus it might change (the user can delete data in her size function for example) and a copy has to be made, which indeed changes the state of the object.  I don't know if there was a better design possible, but maybe I will add this comment as future reference, why the non-const-ness is necessary at the moment.
[3] https://hg.savannah.gnu.org/hgweb/octave/rev/960f4b9a26af#l8.7
[4] https://hg.savannah.gnu.org/hgweb/octave/rev/67fc970dad7d#l9.8
[5] https://hg.savannah.gnu.org/hgweb/octave/rev/67fc970dad7d#l6.16

Kai Torben Ohlhus <siko1056>
Group Member
Wed 03 Oct 2018 12:03:49 AM UTC, comment #2: 

This looks like a noticeable behavior change that might affect user code, if they were relying on the previous (wrong) behavior. So maybe not suitable for the stable branch?

Mike Miller <mtmiller>
Group Member
Tue 02 Oct 2018 11:00:51 PM UTC, comment #1: 

After a while, I hope to come up with a satisfying solution:

The root of a series of issues (bug #44498 "isvector", bug #43925 "isscalar", and this one) is, that Matlab totally relies on "size" to determine the shape of an object, c.f. the documentation of R2018a:


isscalar(A) returns logical 1 (true) if size(A) returns [1 1], and logical 0 (false) otherwise.
isrow   (V) returns logical 1 (true) if size(V) returns [1 n] with a nonnegative integer value n, and logical 0 (false) otherwise.
iscolumn(V) returns logical 1 (true) if size(V) returns [n 1] with a nonnegative integer value n, and logical 0 (false) otherwise.
isvector(A) returns logical 1 (true) if size(A) returns [1 n] or [n 1] with a nonnegative integer value n, and logical 0 (false) otherwise.
ismatrix(V) returns logical 1 (true) if size(V) returns [m n] with nonnegative integer values m and n, and logical 0 (false) otherwise.


Currently, Octave has a mixture of "numel"- and "dims"-calls (the last one not Matlab compatible, not overloadable) to address this functionality.

The proposed patch (file #45128) took advantage of the patches of bug #43925 and suggests the following behavior:


rows    (@var{a}) Return the number of rows    of @var{a}.  This is equivalent to @code{size (@var{a}, 1)}.
columns (@var{a}) Return the number of columns of @var{a}.  This is equivalent to @code{size (@var{a}, 2)}.

isscalar (@var{x}) Return true if @var{x} is a scalar, i.e., @code{size (@var{x})} returns @code{[1 1]}.
isrow    (@var{x}) Return true if @var{x} is a row vector, i.e., @code{size (@var{x})} returns @code{[1 N]} with non-negative N.
iscolumn (@var{x}) Return true if @var{x} is a column vector, i.e., @code{size (@var{x})} returns @code{[N 1]} with non-negative N.
isvector (@var{x}) Return true if @var{x} is a vector.  A vector is a 2-D array where one of the dimensions is equal to 1.  As a consequence a 1x1 array, or scalar, is also a vector.
ismatrix (@var{a}) Return true if @var{a} is a 2-D array, i.e., @code{size (@var{a})} returns @code{[M N]} with non-negative M and N.
issquare (@var{x}) Return true if @var{x} is a square matrix, i.e., @code{size (@var{x})} returns
@code{[N N]} with non-negative N.


The patch resolves two bugs and 10 XFails.  The only remaining XFail is "ndims", because it has an additional predicate "chop trailing singleton dimensions".  I am not sure if "size" does this automatically for me, especially, if a user is in charge of what is returned ;-)  Worst-case: one has to manually post process the output of size.

The patch does not change functionality, it simply uses another function for doing the job, i.e. "size", which is overloadable by user-defined classes.  And the documentation for these functions becomes more precise.

Thus I want to apply it to stable in a few days.

Kai Torben Ohlhus <siko1056>
Group Member
Sat 24 Jun 2017 11:57:04 PM UTC, original submission:  

This problem already has tests in the test/classes/classes.tst file.  If there is a bug report specifically for this issue, I couldn't find it.  But #44334 mentions this problem, but it is closed as fixed, so it seems better to me to open a new report instead of reopen that one.

The test are the xtests from the following blocks in classes.tst.  They require the SizeTester class that is also already a part of the test suite.


%!test st = SizeTester ([1 2]);
%! assert (isequal (size (st), [1 2]));
%! assert (isequal (numel (st), 2));
%!assert (isequal (ndims (st), 2))
%!assert (isequal (rows (st), 1))
%!assert (isequal (columns (st), 2))
%!assert <*44334> (isequal (st, st))
%!assert <44498> (not (isscalar (st)))
%!assert (isvector (st))

%!test st = SizeTester ([2 3]);
%! assert (isequal (size (st), [2 3]));
%! assert (isequal (numel (st), 6));
%!assert (isequal (ndims (st), 2))
%!xtest assert (isequal (rows (st), 2))
%!xtest assert (isequal (columns (st), 3))
%!assert <*44334> (isequal (st, st))
%!assert <44498> (not (isscalar (st)))
%!assert <44498> (not (isvector (st)))

%!test st = SizeTester ([2 3 4]);
%! assert (isequal (size (st), [2 3 4]));
%! assert (isequal (numel (st), 24));
%!xtest assert (isequal (ndims (st), 3))
%!xtest assert (isequal (rows (st), 2))
%!xtest assert (isequal (columns (st), 3))
%!assert <*44334> (isequal (st, st))
%!assert <44498> (not (isscalar (st)))
%!assert <44498> (not (isvector (st)))


I'll tag with a bug number once this report is submitted.

John W. Eaton <jwe>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Carbon-Copy List
  • -email is unavailable- added by siko1056
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by jwe (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 group members can vote.

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-01-24 siko1056 Open/ClosedOpen Closed
    2018-10-04 siko1056 StatusPatch Submitted Fixed
        Release4.4.1 dev
    2018-10-02 siko1056 StatusConfirmed Patch Submitted
        Assigned toNone siko1056
        Releasedev 4.4.1
        Carbon-Copy- Added -email is unavailable-
    2018-10-02 siko1056 Attached File- Added bugs_51308_44498_43925_stable.patch, #45128
    2018-10-02 siko1056 Dependencies- Depends on bugs #44334
    2018-10-02 siko1056 Dependencies- Depends on bugs #44498
        Dependencies- Depends on bugs #43925
    2017-07-22 rik5 Carbon-CopyRemoved 72865 -
    2017-07-22 rik5 StatusNone Confirmed
        Summaryndims, rows, columns in correct for classes that only overload size ndims, rows, columns incorrect for classes that only overload size

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code