bugGNU Octave - Bugs: bug #39041, concatenation of cell array and...

 
 

bug #39041: concatenation of cell array and scalar struct

Submitter:  Clemens Buchacher <drizzd>
Submitted:  Wed 22 May 2013 04:07:01 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  3 - Low Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Clemens Buchacher 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

Mon 22 May 2017 07:46:32 PM UTC, comment #17: 

Done. Also reported bug #51086 for the newly discovered incompatibility. Added test cases for both

https://hg.savannah.gnu.org/hgweb/octave/rev/4d7928872999

Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 07:06:50 PM UTC, comment #16: 

My patch in comment #11 solves the original issue, I can apply that now. It leaves open the issue of this new case


[struct(), {}]


In Matlab this returns an empty scalar struct, in Octave it throws the same error as if they were not empty, concatenation not supported. No better or worse than we are today, so we can look at that separately.

I'll push what I have now since it works (after a make check again).

Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 07:03:15 PM UTC, comment #15: 

So does the second patch in comment #11 resolve the issue?

Rik <rik5>
Group administrator
Mon 22 May 2017 06:51:03 PM UTC, comment #14: 

From the maintainers list, in Matlab 2017a:


>> [cell(1), struct('foo','bar')]

ans =

  1×2 cell array

    []    [1×1 struct]

>> [struct('foo','bar'), cell(1)]
Error using horzcat
The following error occurred converting from cell to struct:
Conversion to struct from cell is not possible.

>> [{}, struct()]

ans =

  cell

    [1×1 struct]

>> [struct(), {}]

ans =

  struct with no fields.


That last one makes me sad. This means that if the first element is a struct and the remaining elements are empty cell arrays, they can be ignored, and the result is a scalar struct. But if any elements are non-empty cell arrays, then the error is thrown.

If the leading element is a cell array, empty or not, then the result is a cell array containing the scalar struct or struct array as a single cell element.

Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 05:45:44 PM UTC, comment #13: 

I traced it back to https://hg.savannah.gnu.org/hgweb/octave/rev/d803d2702a39 in 2011, which references bug #33966 and partially describes

> (tm_row_const::tm_row_const_rep::do_init_element): Check for cells and whether the first element is a struct. Don't check dimensions here.
> (tm_row_const::tm_row_const_rep::init): Convert expressions to values here. Maybe convert list elements to cells. Check dimensions.


That bug report confirms the test case in Matlab (at the time), but I can ask for confirmation in a recent version.

When I hit changes that do a lot of reformatting and I want to trace something back further, I usually use "hg grep --all REGEXP" and let it search the entire project history for a pattern, takes a minute or two but should list the first revision where it was introduced.

Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 05:36:05 PM UTC, comment #12: 

I tried to use 'hg blame' to figure out when this code got introduced, but I could only trace it back to the introduction of the file in this cset:


changeset:   23435:c452180ab672
user:        John W. Eaton <jwe@octave.org>
date:        Fri Apr 21 18:07:40 2017 -0400
summary:     begin refactoring parse tree evaluator


I'm sure it existed in another file before the re-organization and may be an artifact of Matlab behavior from years and years ago.

I agree that someone with access to Matlab should run the tests in comment #8.  @Mike: You could post the code to the octave-maintainer's list.  There's almost always someone willing to run a quick test there.  Just make sure they also include the version of Matlab.



Rik <rik5>
Group administrator
Mon 22 May 2017 05:28:05 PM UTC, comment #11: 

If we do need to preserve this weird case, then the following cleaner change also fixes this issue for me:


diff --git a/libinterp/parse-tree/pt-tm-const.cc b/libinterp/parse-tree/pt-tm-const.cc
--- a/libinterp/parse-tree/pt-tm-const.cc
+++ b/libinterp/parse-tree/pt-tm-const.cc
@@ -82,6 +82,8 @@ namespace octave
             first_elem = false;
           }
       }
+    else if (val.is_cell ())
+      first_elem = false;

     append (val);


This still produces an error with


[struct(), {}]


but now the following works


[{}, struct()]


Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 05:19:05 PM UTC, comment #10: 

And the corresponding unit test case in the parser that fails (by allowing this concatenation to succeed) with this change made:


%!error [struct("foo", "bar"), cell(1)]


Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 05:16:42 PM UTC, comment #9: 

This hack fixes the issue for me, but like I said I am not sure what the intent of this flag is to begin with, it seems like an arcane corner case that it is trying to address


diff --git a/libinterp/parse-tree/pt-tm-const.cc b/libinterp/parse-tree/pt-tm-const.cc
--- a/libinterp/parse-tree/pt-tm-const.cc
+++ b/libinterp/parse-tree/pt-tm-const.cc
@@ -77,7 +77,7 @@ namespace octave
         if (first_elem)
           {
             if (val.is_map ())
-              first_elem_is_struct = true;
+              first_elem_is_struct = false;

             first_elem = false;
           }


Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 04:57:57 PM UTC, comment #8: 

Is the following concatenation not legal in Matlab?


x.a = 1;
y = {'ok'};
z = [x, y];


but reversing the order of concatenation is legal?

That seems to be the special case that the tm_const class is handling with this logic:


    if (any_cell && ! any_class && ! first_elem_is_struct)
      cellify ();


Otherwise I don't understand why that check is in there. If first_elem_is_struct is removed, then the concatenation should work.

Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 04:38:48 PM UTC, comment #7: 

That should be "tm_const", not "tm_struct".

Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 04:32:46 PM UTC, comment #6: 

The problem occurs when the parser is building up the tm_struct object representing the constant values in the matrix expression.

With this expression


[{}, struct()]


the first element seems to be ignored because it's empty, inferred because first_elem_is_struct is set to true, which means that the struct value is not "cellified" on line 313.

With this expression


[{1}, struct()]


the first element is not ignored, first_elem_is_struct is false, and the struct is cellified, which means that the later cell value extractor works.

Mike Miller <mtmiller>
Group Member
Mon 22 May 2017 04:20:59 PM UTC, comment #5: 

I was able to obtain a backtrace for


[ {}, struct() ]




#0  octave_base_value::cell_value (this=0x14d35a0) at libinterp/octave-value/ov-base.cc:549
#1  0x00007f254e16dd6d in octave_value::cell_value (this=this@entry=0x16de4d0)
    at libinterp/octave-value/ov.cc:1649
#2  0x00007f254e230ea9 in octave_value_extract<Cell> (v=...) at libinterp/corefcn/Cell.h:147
#3  octave::single_type_concat<Cell, octave_value> (result=..., dv=..., tmp=...)
    at libinterp/parse-tree/pt-tm-const.h:331
#4  0x00007f254e22cf13 in octave::do_single_type_concat<Cell> (tmp=..., dv=...)
    at libinterp/parse-tree/pt-tm-const.h:424
#5  octave::tree_evaluator::visit_matrix (this=0x12f2720, expr=...)
    at libinterp/parse-tree/pt-eval.cc:1371
#6  0x00007f254e2436fa in octave::tree_matrix::accept (this=<optimized out>, tw=...)
    at libinterp/parse-tree/pt-mat.h:74
#7  0x00007f254e0b52d2 in octave::tree_evaluator::evaluate (this=this@entry=0x12f2720,
    expr=expr@entry=0x14d48a0, nargout=<optimized out>, nargout@entry=0)
    at libinterp/parse-tree/pt-eval.h:254
#8  0x00007f254e2240a5 in octave::tree_evaluator::visit_statement (this=0x12f2720, stmt=...)
    at libinterp/parse-tree/pt-eval.cc:2000
#9  0x00007f254e24c4f0 in octave::tree_statement::accept (this=<optimized out>, tw=...)
    at libinterp/parse-tree/pt-stmt.h:115
#10 0x00007f254e21ef4c in octave::tree_evaluator::visit_statement_list (this=0x12f2720, lst=...)
    at libinterp/parse-tree/pt-eval.cc:2042
#11 0x00007f254e5a064c in octave::tree_statement_list::accept (tw=..., this=<optimized out>)
    at libinterp/parse-tree/pt-stmt.h:193
#12 octave::interpreter::main_loop (this=this@entry=0x12f29d0)
    at libinterp/corefcn/interpreter.cc:946
#13 0x00007f254e5a7393 in octave::interpreter::execute (this=0x12f29d0)
    at libinterp/corefcn/interpreter.cc:670
#14 0x00007f254dc950ad in octave::application::execute_interpreter (this=<optimized out>)
    at libinterp/octave.cc:394
#15 0x00007f254dc94ca8 in octave::cli_application::execute (this=this@entry=0x7fff350694a0)
    at libinterp/octave.cc:446
#16 0x0000000000401213 in main (argc=8, argv=0x7fff350696f8) at src/main-cli.cc:90


As expected, Octave gets it right that the return type for this should be a cell array.  It calls do_single_type_concat<Cell> correctly.  The probablem seems to be that the struct is held as an octave_value.  When the code tries to get the underlying object using octave_value_extract<Cell> this results in calling the method cell_value() on the octave_value object.  But I don't see an overload in ov-struct.h for the cell_value routine.


Rik <rik5>
Group administrator
Mon 22 May 2017 04:16:45 PM UTC, comment #4: 

I'm adding jwe to the CC list since this is pretty deep in the internals.

Rik <rik5>
Group administrator
Sun 20 Nov 2016 10:32:56 PM UTC, comment #3: 

This behavior is still present in Octave 4.2.0.

Hartmut <hardy>
Wed 31 Dec 2014 05:29:32 PM UTC, comment #2: 

Also of note, Octave succeeds if the concatenation is along the first dimension (rows) rather than along the column dimension.


[ {}; struct() ]   # works
[ {}, struct() ]   # fails


That kind of inconsistency isn't great.

Rik <rik5>
Group administrator
Mon 24 Jun 2013 12:59:13 AM UTC, comment #1: 

Confirmed on a recent development tip (6/23/13).  There is a work-around so I'm going to lower the priority.

Rik <rik5>
Group administrator
Wed 22 May 2013 04:07:01 PM UTC, original submission:  

This is legal in Matlab R2011b

>> [{}, struct()]


ans =

    [1x1 struct]

but not in Octave:

octave:1> [{}, struct()]
error: octave_base_value::cell_value(): wrong type argument 'scalar struct'

Octave is consistent for other scalar or array types:

octave:2> [{}, 1]
ans =
{
  [1,1] =  1
}
octave:3> [{}, [1, 2]]
ans =
{
  [1,1] =

     1   2

}

A workaround is to enclose the scalar struct in parentheses:

octave:1> [{}, {struct()}]
ans =
{
  [1,1] =

    scalar structure containing the fields:


}

Clemens Buchacher <drizzd>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by rik5
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by mtmiller (Updated the item)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by drizzd (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-05-22 mtmiller StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2017-05-22 rik5 Carbon-Copy- Added jwe
    2017-05-22 mtmiller Dependencies- bugs #51081 is dependent
    2016-03-31 mtmiller Release3.6.4 dev
        Operating SystemMicrosoft Windows Any
    2013-06-24 rik5 Priority5 - Normal 3 - Low
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code