bugGNU Octave - Bugs: bug #65876, error retrieving data from struct...

 
 

bug #65876: error retrieving data from struct values in containers.Maps

Submitter:  A.R. Burgers <arb>
Submitted:  Thu 13 Jun 2024 09:21:54 AM UTC
   
 
Category:  Classdef Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * stable Release: 
Operating System:  * Any Fixed Release:  10.1.0
Planned Release:  10.1.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 24 Jul 2024 01:20:34 AM UTC, comment #17: 

No issues reported.  Marking item as Fixed.

Rik <rik5>
Group administrator
Fri 05 Jul 2024 05:11:37 PM UTC, comment #16: 

No further comments in the last two weeks. So, I went ahead and pushed the change to the default branch:
https://hg.savannah.gnu.org/hgweb/octave/rev/21a9e15bcb54

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Thu 20 Jun 2024 12:33:51 PM UTC, comment #15: 

Maybe I didn't convey what I meant as a non-native speaker, and something was lost in translation, certainly didn't intend to say anything derogatory.

I think your proposal is excellent, and I hope it makes its way into the main code base

A.R. Burgers <arb>
Thu 20 Jun 2024 12:20:48 PM UTC, comment #14: 

I'm happy this is working for you.

But why do you think it is a hack? I was hoping we could actually use that (at least as a first step to introduce `numArgumentsFromSubscript` in Octave).

Markus Mützel <mmuetzel>
Group administrator
Thu 20 Jun 2024 08:52:43 AM UTC, comment #13: 

that's a nifty hack.

Applied it to bytecode-interpreter, and works well on initial use.

A.R. Burgers <arb>
Sun 16 Jun 2024 11:58:04 AM UTC, comment #12: 

Found a few issues in the previous patch (left-overs from v1, warnings from txi syntax, ...).
New patch is attached.



(file #56167)

Markus Mützel <mmuetzel>
Group administrator
Fri 14 Jun 2024 02:55:07 PM UTC, comment #11: 

The attached patch implements support for `numArgumentsFromSubscript` in overloaded `subsref` functions.

The Matlab documentation says that its return value would also be "passed to overloaded subsasgn". But I don't know what that means.
If someone understands that or has an example, we could probably try to implement that part in a later change.

I also don't know what the third input argument of `numArgumentsFromSubscript` is. For the time being, it is just an empty matrix. It should be one of three enumeration values:
https://www.mathworks.com/help/matlab/ref/numargumentsfromsubscript.html#butdgbh-1-indexingContext

This fixes the original example and the test suite still passes for me.


(file #56162)

Markus Mützel <mmuetzel>
Group administrator
Fri 14 Jun 2024 09:04:10 AM UTC, comment #10: 

Found the following in the Matlab documentation:
https://www.mathworks.com/help/matlab/ref/numargumentsfromsubscript.html

We'd probably need to implement support for `numArgumentsFromSubscript` and overload it in containers.Map for this to work correctly...

Markus Mützel <mmuetzel>
Group administrator
Fri 14 Jun 2024 08:24:25 AM UTC, comment #9: 

With this slightly changed version of the classdef:

classdef foocls2
  methods
    function varargout = subsref (obj, idx)
      nout = nargout
      varargout = ...
        num2cell (struct ('x', num2cell (zeros (size (idx(1).subs{1})))));
    end
  end
end


I'm seeing the following results in Matlab R2024a:

>> x(1:3).x
nout =
     3

ans =
  struct with fields:
    x: 0

ans =
  struct with fields:
    x: 0

ans =
  struct with fields:
    x: 0

>> x('ab').x
nout =
     2

ans =
  struct with fields:
    x: 0

ans =
  struct with fields:
    x: 0

>>


The second expression would probably break with the patch from comment #5. (But I haven't actually tested that.)

Markus Mützel <mmuetzel>
Group administrator
Thu 13 Jun 2024 07:57:50 PM UTC, comment #8: 

R2019b gives (some white space removed)


ans = 10
ans = 0
ans = 0
ans = 0
ans = 0
ans = 0
ans = 0
ans = 0
ans = 0
ans = 0
ans = 0


A.R. Burgers <arb>
Thu 13 Jun 2024 05:42:29 PM UTC, comment #7: 

That we are having to consider so many special cases leads me to think we are doing something wrong when trying to evaluate these kinds of expressions.  Maybe there is a better way?

Using the following classdef based on the original example from bug #48693,


classdef foocls
  methods
    function varargout = subsref (obj, idx)
      nargout
      varargout = num2cell (zeros (size (idx(1).subs{1})));
    end
  end
end


what does Matlab display for nargout inside the subsref method when evaluating


x = foocls;
x{1:10}


and what is displayed for the output of the "x{1:10}" indexing expression?

John W. Eaton <jwe>
Group administrator
Thu 13 Jun 2024 04:01:05 PM UTC, comment #6: 

I should have made clearer that the last example shows the results in Matlab R2024a.

Markus Mützel <mmuetzel>
Group administrator
Thu 13 Jun 2024 03:59:36 PM UTC, comment #5: 

I ended up at the same code block.

I'm guessing this block is trying to handle indexing expressions like the following:

a(1).b = 1;
a(2).b = 2;
a(3).b = 3;

c = [a([1,3]).b];


Where the expression inside the bracket in the last line returns a comma-separated list. (Of course, that example doesn't involve overloaded subsref methods. But I hope you get the idea.)

Afaict, the original change that added logic for these kinds of expressions might have been:
https://hg.savannah.gnu.org/hgweb/octave/rev/a18897e4c7b5

The attached patch fixes the issue for me by not interpreting character arrays as valid indices.

However, that is probably not correct in every case. Character arrays can be used as indexing expressions. E.g.:

>> a('ab') = 1;
>> size(a)
ans =
     1    98
>> sum(a)
ans =
     2


Another example on Matlab R2024a:

>> clear a
>> a('a').b = 1;
>> a('b').b = 2;
>> a('ab').b
ans =
     1

ans =
     2


A slightly changed version of the original example doesn't interpret the character vector as indices:

>> cm = containers.Map;
>> data.x = 1;
>> cm('foo') = data;
>> cm('foo').x
ans =
     1


I'm not sure how to reliably detect at that point that the expression inside the parenthesis won't lead to a comma-separated list being required as the result...


(file #56160)

Markus Mützel <mmuetzel>
Group administrator
Thu 13 Jun 2024 02:40:57 PM UTC, comment #4: 

The trouble seems to be due to this block of code in octave_classdef::subsref in ov-classdef.cc:


          // If the number of output arguments is unknown, attempt to set up
          // a proper value for nargout at least in the simple case where the
          // cs-list-type expression - i.e., {} or ().x, is the leading one.
          if (nargout <= 0)
            {
              bool maybe_cs_list_query = (type[0] == '.' || type[0] == '{'
                                          || (type.length () > 1 && type[0] == '('
                                              && type[1] == '.'));

              if (maybe_cs_list_query)
                {
                  // Set up a proper nargout for the subsref call by calling numel.
                  octave_value_list tmp;
                  int nout;
                  if (type[0] != '.') tmp = idx.front ();
                  nout = xnumel (tmp);
                  if (nargout != 0 || nout > 1)
                    nargout = nout;
                }
              else if (nargout < 0)
                nargout = 1;
            }


that was added in changeset https://hg.savannah.gnu.org/hgweb/octave/rev/554a932fc6d0 in an attempt to fix bug #63841.

Although there were some new tests committed for that bug in changeset https://hg.savannah.gnu.org/hgweb/octave/rev/768df05d04f3, none of them fail if I comment out the above code block and the example code from this bug report works again.  I don't understand the point of the above code block.  What problem was it trying to fix?

John W. Eaton <jwe>
Group administrator
Thu 13 Jun 2024 02:25:25 PM UTC, comment #3: 

The example code appears to fail for me with the bytecode interpreter enabled.  I see the following:


octave:1> __vm_enable__
ans = 1
octave:2> __octave_config_info__ ('hg_id')
ans = 6f2baadfdb3d
octave:3>  np = 11;
octave:4> data.x = linspace(0, 10, np)';
octave:5> data.y = rand(np,1);
octave:6> cm = containers.Map;
warning: auto-compilation of Map failed with message classdef constructors are not supported by the VM yet
octave:7> cm('el1') = data;
octave:8> cm('el1')
ans =

  scalar structure containing the fields:

    x =

        0
        1
        2
        3
        4
        5
        6
        7
        8
        9
       10

    y =

       0.7512
       0.9831
       0.8208
       0.5483
       0.8630
       0.2361
       0.4807
       0.2117
       0.7103
       0.6513
       0.6643


octave:9> plot(cm('el1').x, cm('el1').y);
error: subsref: function called with too many outputs
octave:10> cm('el1').x
error: subsref: function called with too many outputs


The

John W. Eaton <jwe>
Group administrator
Thu 13 Jun 2024 10:58:29 AM UTC, comment #2: 

yes, this fails


octave-10.0.0 --norc --quiet --persist --eval '__vm_enable__(0);tst'


this works:


octave-10.0.0 --norc --quiet --persist --eval '__vm_enable__(1);tst'


A.R. Burgers <arb>
Thu 13 Jun 2024 10:05:17 AM UTC, comment #1: 

actually this (the VM) seems to work:


octave-10.0.0 --norc --quiet --persist tst.m


A.R. Burgers <arb>
Thu 13 Jun 2024 09:21:54 AM UTC, original submission:  

The script below fails in octave with:


error: subsref: function called with too many outputs
error: called from
    subsref
    tst at line 6 column 1


it does work in matlab R2019B


np = 11;
data.x = linspace(0, 10, np)';
data.y = rand(np,1);
cm = containers.Map;
cm('el1') = data;
plot(cm('el1').x, cm('el1').y);


fails for me with these versions:


GNU Octave Version: 9.1.91 (hg id: 97b44c6e3e35)
GNU Octave License: GNU General Public License
Operating System: Linux 5.15.0-106-generic #116-Ubuntu SMP Wed Apr 17 09:17:56 UTC 2024 x86_64



GNU Octave Version: 10.0.0 (hg id: 6f2baadfdb3d)
GNU Octave License: GNU General Public License
Operating System: Linux 5.15.0-106-generic #116-Ubuntu SMP Wed Apr 17 09:17:56 UTC 2024 x86_64


A.R. Burgers <arb>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #56167:  bug65876-classdef-subsref-keys-v3.patch added by mmuetzel (7KiB - application/octet-stream)
file #56162:  bug65876-classdef-subsref-keys-v2.patch added by mmuetzel (8KiB - application/octet-stream)
file #56160:  bug65876-classdef-subsref-keys.patch added by mmuetzel (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by arb (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-07-24 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 10.1.0
    2024-07-05 mmuetzel CategoryInterpreter Classdef
    2024-07-05 mmuetzel StatusPatch Submitted Ready For Test
    2024-06-16 mmuetzel Attached File- Added bug65876-classdef-subsref-keys-v3.patch, #56167
    2024-06-14 mmuetzel Attached File- Added bug65876-classdef-subsref-keys-v2.patch, #56162
        StatusNone Patch Submitted
        Planned ReleaseNone 10.1.0
    2024-06-13 mmuetzel Attached File- Added bug65876-classdef-subsref-keys.patch, #56160

    Back to the top

    Powered by Savane 3.14-7003.
    Corresponding source code