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
|
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).
|
Mon 22 May 2017 07:03:15 PM UTC, comment #15:
So does the second patch in comment #11 resolve the issue?
|
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.
|
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.
|
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.
|
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
|
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:
|
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
|
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.
|
Mon 22 May 2017 04:38:48 PM UTC, comment #7:
That should be "tm_const", not "tm_struct".
|
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.
|
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.
|
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.
|
Sun 20 Nov 2016 10:32:56 PM UTC, comment #3:
This behavior is still present in Octave 4.2.0.
|
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.
|
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.
|
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:
}
|