bugGNU Octave - Bugs: bug #46571, classdef numel method not...

 
 

bug #46571: classdef numel method not consulted for cell-indexed assignment

Submitter:  Stéphane Adjemian <step4n>
Submitted:  Mon 30 Nov 2015 01:38:57 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Stéphane Adjemian 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

Wed 02 May 2018 05:09:24 PM UTC, comment #18: 

Ok, then can you please file a new bug?

This bug fix hasn't made things any better or worse for the CasADi example, it's actually completely unrelated. The example you have posted runs the same in Octave 4.0 or 4.2 or 4.4.

The bug you want to report is that cellfun does not correctly find function overloads on objects when the argument is the name of a function (e.g. 'numel').

Mike Miller <mtmiller>
Group Member
Wed 02 May 2018 06:26:52 AM UTC, comment #17: 

Joris (CasADi developer) provides this SSCCE of the bug - 

classdef Foo

methods
  function r=size(self)
    r = [2 3];
  end

  function r=numel(self)
    r = 6;
  end
end
end
numel(Foo) % 6
cellfun('numel',{Foo Foo}) % [1 1]

Joris also mentions:

"I think the classdef.cc needs some love..
You can refer to https://savannah.gnu.org/bugs/?46571, which is similar, and whose patch perhaps would have already fixed it if the added numel were virtual."

deego <deego>
Wed 02 May 2018 06:20:07 AM UTC, comment #16: 

Mike,

Pardon me, should have mentioned. I mean that the bug is present in 4.2.2 (from debian backports).


(I'm unable to test casadi with any other version).


deego <deego>
Wed 02 May 2018 06:17:34 AM UTC, comment #15: 

Do you mean that this bug affected casadi with octave version 4.2.1 or older? And that it is fixed now with octave 4.2.2 or 4.4.0? Thanks.

Mike Miller <mtmiller>
Group Member
Wed 02 May 2018 06:14:05 AM UTC, comment #14: 


Hi,

Just wanted to mention that this bug also affects casadi. (see casadi bug report https://github.com/casadi/casadi/issues/2253 )

Casadi developers confirm that the problem lies with octave, and that matlab doesn't suffer from the same problem. They specifically referred to this bug.

Dave

deego <deego>
Wed 25 Apr 2018 03:40:20 PM UTC, comment #13: 

Yes, that's right.

Mike Miller <mtmiller>
Group Member
Wed 25 Apr 2018 02:15:36 PM UTC, comment #12: 

This piece of code is working on octave 4.0.3 but not on octave 4.2.2. I suspect this discussion is related to the issue.

Consider the class LP.m:

```
classdef LP

properties
    eqs
    A
end

methods
    function problem = LP(eqs)
        problem.eqs = eqs;
    end
    function n = numel(problem)
        n = numel(problem.eqs);
    end
end
end
```

If I run the script:

>> p = LP(zeros(2,1));
>> p.A = 1;


I get:

error: invalid assignment to cs-list outside multiple assignment


Andres Codas <andrescodas>
Tue 27 Feb 2018 11:30:06 PM UTC, comment #11: 

I've pushed this on the stable branch (for the upcoming 4.2.2 release) and merged to default. Thanks for your patch and your patience!

https://hg.savannah.gnu.org/hgweb/octave/rev/7605e06a50d5

Mike Miller <mtmiller>
Group Member
Wed 21 Feb 2018 11:05:03 PM UTC, comment #10: 

Thanks to some compatibility tests, pytave's numel will be changing, so this won't be a problem. So I'm in favor of applying this to stable for the upcoming point release. I'll wait a little while to see if anyone has any more tests or objections before pushing it, but it will be great to have this long standing bug fixed. Thanks for the work Sébastien!

Mike Miller <mtmiller>
Group Member
Wed 21 Feb 2018 09:21:23 PM UTC, comment #9: 

I've tested the updated patch and it also applies cleanly to stable, no test suite problems, the new test passes, but still breaks pytave.

I'll post another update after I've seen whether this is a problem I can work around in pytave's numel / subsasgn or not.

Mike Miller <mtmiller>
Group Member
Wed 21 Feb 2018 08:09:14 PM UTC, comment #8: 

I found the problem with my previous patch: it was incorrectly testing whether in a built-in or class-method context.

I attach a new version of the patch that fixes this issue. Hopefully it also fixes the pytave issues.

The new changeset also contains a test case for the bug.

Concerning the numArgumentsFromSubscript method: it was introduced in MATLAB R2015b, and offers an alternative way to override the number of arguments to subsref/subsasgn. It could be implemented as a separate, independent, patch.

(file #43381)

Sébastien Villemot <svillemot>
Wed 21 Feb 2018 06:51:21 PM UTC, comment #7: 

Thanks for correcting me, it's been a while since I've looked at classdef. Right, overriding numel works, but only at the interpreter level, certain Octave internal operations use the builtin definition of numel which is 1 for handle classes.

Mike Miller <mtmiller>
Group Member
Wed 21 Feb 2018 11:58:59 AM UTC, comment #6: 

Thanks Mike.

Actually it's already possible, without this patch, to override the numel method for classdef.

What this patch does is to tie the C++ numel method to the user override, which is not quite the same thing.

The problem with my patch is that, while it fixes some indexing behaviours, it breaks others, as you noted.

So please do not apply as it is, I am going to work on a better solution.

Sébastien Villemot <svillemot>
Wed 21 Feb 2018 08:22:50 AM UTC, comment #5: 

I've verified that Sébastien's patch applies and introduces no regressions in the test suite, but a test case would be good (as we talked about on irc).

As we also talked about on irc, this may not be a complete solution, Sébastien mentioned looking at the Matlab object method numArgumentsFromSubscript, which I think is supposed to override how nargin and nargout are interpreted for subsref and subsasgn.

I'll also note that this change does break some indexing behavior that I am relying on in pytave. If we do apply this patch, on either the stable or default branch, I would appreciate some help in figuring out how to get pytave subsasgn working again.

I do think this patch is worth applying ultimately, even if it's not a complete solution to the overall class indexing problem. It definitely should be possible for a user class to override numel, which isn't possible today.

Mike Miller <mtmiller>
Group Member
Fri 24 Nov 2017 08:18:16 PM UTC, comment #4: 

Thanks to the analysis done by Mike Miller, I was able to craft a patch for this issue (in attachment).

It adds the octave_classdef::numel method, tying it to the user override method.

The patch applies on the stable branch. I verified that it introduces no regression in the testsuite (I also tested it on the default branch).


(file #42472)

Sébastien Villemot <svillemot>
Fri 05 Aug 2016 12:46:57 AM UTC, comment #3: 

Dug into this a bit further, and confirmed that the error is that the numel method is not consulted when Octave is trying to figure out how many values need to be on the left and right of the assignment operator in an indexing expression. Since the inconsistency is detected solely through calling the numel method, the class subsasgn is never even called.

Octave expects types to define a numel function that returns the appropriate number of elements that would be indexed given an indexing argument. This is done correctly for old-style classes but not classdef classes.

Therefore, as assignment like `a{'v1'} = ones (10, 1)` is assumed to be assigning to 2 elements of a cell-array-like thing.

Mike Miller <mtmiller>
Group Member
Tue 02 Aug 2016 07:53:09 AM UTC, comment #2: 

Actually I think the error is occurring in the parser, but I am not sure why it is triggered for new-style classes but not old-style classes.

With just a trivial class definition / constructor and no other methods defined, this error occurs with classdef:


@someclass/someclass.m:

classdef someclass < handle
endclassdef

>> x = someclass;
>> x{'foo'} = 0
error: invalid assignment to cs-list outside multiple assignment


but not if the class is a struct-based old-style class:


@someclass/someclass.m:

function y = someclass ()
  y = class (struct (), "someclass");
endfunction

>> x = someclass;
>> x{'foo'} = 0
error: class cannot be indexed with {


In the first case, the error is caught when the parser is evaluating the assignment expression. In the second case, the error is only because there is no custom aubsasgn method defined.

You're right that it has something to do with interpreting the string index as an array of multiple indices, but I'm not clear at the moment why it's not a problem when the old class definition style is used.

Mike Miller <mtmiller>
Group Member
Fri 15 Apr 2016 12:18:49 AM UTC, comment #1: 

If the code doesn't enter @myclass/subsasgn, it will be interpreting 'v1' as 1x2 array, which is why it gets confused about a cs-list.

The fact that it "works" with just 'v' is probably a result of the fact that the custom subsasgn does the same thing as the default subsasgn in that case.

Lachlan Andrew <lachlan>
Mon 30 Nov 2015 01:38:57 PM UTC, original submission:  

Calls to the subsasgn are not working with the new class syntax. In the attached files I provide two implementations of the same class with the old and new (ie using classdef) interfaces.

With the new interface the following code :


a = myclass(data(10,2), {'v1'; 'v2'});
a{'v1'}=ones(10,1)


returns


error: invalid assignment to cs-list outside multiple assignment


while the same code returns the expected results with the old oop syntax. Note also that both interfaces produce the same expected results with Matlab 2015b. Debugging the code reveals that we never enter the @myclass/subsasgn function.

Note also that the problem does not pop up if the length of the string used as an index is equal to one, ie:


a = myclass(data(10,2), {'v1'; 'v2'});
a{'v'}=ones(10,1)


produces the expected result.

Stéphane Adjemian <step4n>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43381:  bug46571-v2.patch added by svillemot (4KiB - text/x-patch)
file #42472:  bug46571.patch added by svillemot (2KiB - text/x-patch)
file #35596:  new-syntax.tar added by step4n (10KiB - application/x-tar - Two implementations of the same class)
file #35597:  old-syntax.tar added by step4n (10KiB - application/x-tar - Two implementations of the same class)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by deego (Posted a comment)
  • -email is unavailable- added by andrescodas (Posted a comment)
  • -email is unavailable- added by avinoam (Updated the item)
  • -email is unavailable- added by svillemot (Updated the item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by step4n (Submitted the item)
  • -email is unavailable- added by step4n
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2018-02-27 mtmiller StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2018-02-21 mtmiller StatusPatch Submitted Patch Reviewed
    2018-02-21 svillemot Attached File- Added bug46571-v2.patch, #43381
    2018-02-19 avinoam StatusConfirmed Patch Submitted
    2017-11-24 svillemot Attached File- Added bug46571.patch, #42472
    2016-08-05 mtmiller SummaryInconsistency between old and new oop syntax (wrt subsasgn) classdef numel method not consulted for cell-indexed assignment
    2016-08-02 mtmiller StatusNone Confirmed
        Release4.0.0 dev
    2016-04-15 lachlan Item GroupNone Incorrect Result
    2015-11-30 step4n Attached File- Added new-syntax.tar, #35596
        Attached File- Added old-syntax.tar, #35597
        Carbon-Copy- Added step4n

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code