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

 
 

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

Submitted by:  John W. Eaton <jwe>
Submitted on:  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:  Kai Torben Ohlhus <siko1056>
Originator Name:  jwe Open/Closed:  Open
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.

 

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>
Project MemberIn charge of this item.
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>
Project 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:

Proposed:

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>
Project MemberIn charge of this item.
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>
Project Administrator
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:

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:

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>
Project MemberIn charge of this item.
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.

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

John W. Eaton <jwe>
Project Administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

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

    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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    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.3