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

 
 

bug #39041: concatenation of cell array and scalar struct

Submitted by:  Clemens Buchacher <drizzd>
Submitted on:  Wed 22 May 2013 04:07:01 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 3 - LowItem Group: Matlab Compatibility
Status: FixedAssigned to: None
Originator Name: Clemens BuchacherOpen/Closed: Closed
Release: devOperating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project Administrator
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

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>
Project Administrator
Mon 22 May 2017 07:03:15 PM UTC, comment #15:

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

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

From the maintainers list, in Matlab 2017a:

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>
Project Administrator
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>
Project Administrator
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:

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>
Project 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:

This still produces an error with

but now the following works

Mike Miller <mtmiller>
Project Administrator
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:

Mike Miller <mtmiller>
Project Administrator
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

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

Is the following concatenation not legal in Matlab?

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:

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>
Project Administrator
Mon 22 May 2017 04:38:48 PM UTC, comment #7:

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

Mike Miller <mtmiller>
Project Administrator
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

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

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>
Project Administrator
Mon 22 May 2017 04:20:59 PM UTC, comment #5:

I was able to obtain a backtrace for

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>
Project 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>
Project 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.

That kind of inconsistency isn't great.

Rik <rik5>
Project 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>
Project 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 File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by rik5
  • -unavailable- added by hardy (Posted a comment)
  • -unavailable- added by mtmiller (Updated the item)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by drizzd (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 22 May 2017 07:46:32 PM UTCmtmillerStatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
    Mon 22 May 2017 04:16:45 PM UTCrik5Carbon-Copy-=>Added jwe
    Mon 22 May 2017 03:37:51 PM UTCmtmillerDependencies-=>bugs #51081 is dependent
    Thu 31 Mar 2016 05:26:24 AM UTCmtmillerRelease3.6.4=>dev
      Operating SystemMicrosoft Windows=>Any
    Mon 24 Jun 2013 12:59:13 AM UTCrik5Priority5 - Normal=>3 - Low
      StatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1