bugGNU Octave - Bugs: bug #56167, non-existent cell indexing 'C{}'...

 
 

bug #56167: non-existent cell indexing 'C{}' should produce an error

Submitter:  SillyMon <sillymon>
Submitted:  Fri 19 Apr 2019 01:41:08 AM UTC
   
 
Category:  Interpreter Severity:  2 - Minor
Priority:  3 - Low Item Group:  Missed Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  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 24 Apr 2019 03:13:15 PM UTC, comment #23: 
Rik <rik5>
Group administrator
Wed 24 Apr 2019 05:43:58 AM UTC, comment #22: 

A small typo was found in changeset https://hg.savannah.gnu.org/hgweb/octave/rev/ffdfeb835aef

Here is a patch for this:


--- a/scripts/help/warning_ids.m
+++ b/scripts/help/warning_ids.m
@@ -174,7 +174,7 @@
 ## enabled.
 ##
 ## @item Octave:empty-index
-## If the @code{Octave:index} warning is enabled then Octave will emit a
+## If the @code{Octave:empty-index} warning is enabled then Octave will emit a
 ## warning whenever indexing operators are used without an index, for example
 ## @code{@var{x}()}.
 ## By default, the @code{Octave:empty-index} warning is enabled.


Anonymous
Tue 23 Apr 2019 09:10:30 PM UTC, comment #21: 

For completeness, I documented the new warning ID in warning_ids.m.  See https://hg.savannah.gnu.org/hgweb/octave/rev/ffdfeb835aef.

Rik <rik5>
Group administrator
Tue 23 Apr 2019 06:49:47 PM UTC, comment #20: 

I pushed the following changeset so now we should see warnings about indexing values with ():

http://hg.savannah.gnu.org/hgweb/octave/rev/d61825e693f2

John W. Eaton <jwe>
Group administrator
Tue 23 Apr 2019 06:22:06 PM UTC, comment #19: 

Warnings can be disabled in smaller regions of code by using the "local" argument to warning() as in:


warning ("off", "WARNING_ID", "local")


From the help text,


If a final argument "local" is provided then the warning state will
be set temporarily until the end of the current function.  Changes
to warning states that are set locally affect the current function
and all functions called from the current scope.  The previous
warning state is restored on return from the current function.  The
"local" option is ignored if used in the top-level workspace.


I think it is an advantage to warn the programmer about potentially confusing accessing a variable versus calling a function, so I would vote against supporting uniform access.

In any case, Matlab has already chosen a direction.  They emit a warning today, and will make it an error to use this syntax in the future, so it wouldn't be bad to be moving in the same direction.


Rik <rik5>
Group administrator
Tue 23 Apr 2019 04:07:26 PM UTC, comment #18: 


> Since empty indexing '()' of regular objects is likely to indicate a coding problem, could we emit a warning when that is used? It could be a warning_with_id() so that the whole mechanism could be switched off easily if a programmer has used the construct extensively.


Hmm. I dunno about that. If the purpose of supporting this indexing form is to allow programmers to use the "uniform access principle" and interchange function calls and array indexing, then it probably shouldn't generate a warning in one of those cases. And warnings can only be disabled globally, which is kind of course-grained.

Maybe it could be a warning whose state defaults to off, allowing developers to enable it only when they want to do stricter debugging?

Andrew Janke <apjanke>
Tue 23 Apr 2019 03:32:31 PM UTC, comment #17: 

@jwe: I tested this and it works fine


octave:1> x = {1, 2; 3, 4}
x =
{
  [1,1] =  1
  [2,1] =  3
  [1,2] =  2
  [2,2] =  4
}

octave:2> x()
ans =
{
  [1,1] =  1
  [2,1] =  3
  [1,2] =  2
  [2,2] =  4
}

octave:3> x{}
error: invalid empty index expression


One final idea.  Since empty indexing '()' of regular objects is likely to indicate a coding problem, could we emit a warning when that is used?  It could be a warning_with_id() so that the whole mechanism could be switched off easily if a programmer has used the construct extensively.  Otherwise, it would be a good note to the programmer to change the one or two instances of their code that use this.

Rik <rik5>
Group administrator
Tue 23 Apr 2019 11:26:03 AM UTC, comment #16: 

Guillaume: Thanks.  Do you get the same warning for numeric objects or just cell arrays?

I also noticed that if I make x() error for cell arrays, a containers.Map test fails.  For example


m = containers.Map (realmax ("double"), pi)
m.keys ()


fails in the subsref method for Map where it does where it does 


sref = subsref (sref, s(2:end));


At that point, s is a 1x2 struct array with type => '.', '()'  and subs => "keys", {} so the () part fails because the index is empty.

John W. Eaton <jwe>
Group administrator
Tue 23 Apr 2019 11:13:03 AM UTC, comment #15: 

I pushed the following changeset.  If it's not the correct thing to do, then please explain the requirements for compatibility here and I'll try to come up with a better fix.

http://hg.savannah.gnu.org/hgweb/octave/rev/3140380861ce

John W. Eaton <jwe>
Group administrator
Tue 23 Apr 2019 10:55:28 AM UTC, comment #14: 

x() returns x for built-in arrays objects but in recent Matlab versions there is now a warning discouraging its use:


>> x = {1, 2; 3, 4}
x =
  2x2 cell array
    {[1]}    {[2]}
    {[3]}    {[4]}
>> x()
Warning: A value of class "cell" was indexed with no subscripts specified. Currently the result of this operation is
the indexed value itself, but in a future release, it will be an error.
ans =
  2x2 cell array
    {[1]}    {[2]}
    {[3]}    {[4]}


Guillaume <gyom>
Tue 23 Apr 2019 10:28:24 AM UTC, comment #13: 

I'm not sure why x() and X{} are allowed for built-in array objects.  Stranger still is that x{} works like x{:} for cell array objects.

It does make sense to call subsref for those expressions, but for numeric objects I think they should be errors.

I noticed there is a test in Cell.cc to ensure that "a() == a" for cell array objects and that containers.Map relies on that behavior.  What does Matlab do for


x = {1, 2; 3, 4}
x()


?

John W. Eaton <jwe>
Group administrator
Fri 19 Apr 2019 05:56:42 PM UTC, comment #12: 

I separated the concatenation issue to its own bug report (see bug #56172).

Rik <rik5>
Group administrator
Fri 19 Apr 2019 05:26:52 PM UTC, comment #11: 

I agree with everything Rik just said.

Andrew Janke <apjanke>
Fri 19 Apr 2019 05:23:52 PM UTC, comment #10: 

This bug is related to bug #31615 which I have added as a dependency.

Rik <rik5>
Group administrator
Fri 19 Apr 2019 05:12:58 PM UTC, comment #9: 

The syntax of indexing without providing an actual index is not a good coding practice.  First, it is unclear what is intended and it relies on a specific side effect that empty indexing '{}' is equivalent to colon indexing '{:}'.  Better to simply write '{:}' if that is what the code should do.  Second, it makes the code platform-specific to Octave.  Matlab does not support this syntax.

The Octave motto is "Free Your Numbers" because you should be able to run code you write on whatever OS (Linux, Mac, Windows) you want and with whatever interpreter you like (Octave, Matlab).  Writing code that will only work with an Octave side effect seems like it is a step backwards.

So, I agree with Andrew that Octave should produce an error for this syntax.  I chaged the title to reflect that.

The second question is whether Octave should allow concatenation of empty arrays that differ in size.  When the arrays are not-empty this clearly is an error


[zeros(2,1), zeros(2,1), 1]
error: horizontal dimensions mismatch (2x2 vs 1x1)


But if they are empty, should they just be ignored completely?


octave:1> [zeros(0,1), zeros(0,1), 1]
error: horizontal dimensions mismatch (0x2 vs 1x1)


Not that we have to follow Matlab, but they currently allow this syntax, but warn that is will be removed.

The warning is


Warning: this concatenation operation includes an empty array with an incorrect number of rows.
Concatenation including empty arrays will require all arrays to have the same number of rows in a future release.


I'm guessing it might be good for Octave to support this, with a warning like Matlab, because there is probably m-file code developed over the last 25 years that expects this behavior.

Rik <rik5>
Group administrator
Fri 19 Apr 2019 04:46:04 PM UTC, comment #8: 

I am not very experienced with what makes a bug more or less sever or important.

Between a obtuse error message and a bug in the basic functionality
that possibly only occurs for some specific unfortunate input but
must be considered in all code written with dynamic indexing of
arrays, the bread and butter of Octave. The last one is definitely
very high on my list of things that would make it difficult to
recommend Octave.

SillyMon <sillymon>
Fri 19 Apr 2019 02:25:37 AM UTC, comment #7: 


> So, one choice would be to make Octave compatible with Matlab and error for this usage.


I vote for this. I don't see an advantage for the empty-index usage, and compatibility is a nice default.

Plus, I think there's a good chance that "c{}" indexing is just a typo. E.g. this:


[c{}]


Might be just a typo for this:


[c {}]


which is a way of concatenating an empty cell onto an existing array.

Andrew Janke <apjanke>
Fri 19 Apr 2019 02:23:52 AM UTC, comment #6: 


> [ones(0,1) ones(0,1) 1]
>
> same error


That's just another way of doing the same operation; both end up calling horzcat() or cat() on the same values. [...] horizontally concatenates a comma-separated list; {}-indexing a cell produces a comma-separated list.

Andrew Janke <apjanke>
Fri 19 Apr 2019 02:20:44 AM UTC, comment #5: 


[ones(0,1) ones(0,1) 1]

same error

SillyMon <sillymon>
Fri 19 Apr 2019 02:13:01 AM UTC, comment #4: 

Confirmed.  I have re-titled the bug report to describe what I believe is the error.

Note, this code does not run under Matlab.  Attempting to run this section


tmp{1} = 1;
tmp{2} = ones(3)(logical(zeros(3)));
tmp{3} = ones(3)(logical(zeros(3)));
tmp{4} = 1;
[tmp{}]


results in an error at the last line about an invalid expression.  In fact, any indexing without an actual index is an error in Matlab.  For example,


tmp{}


So, one choice would be to make Octave compatible with Matlab and error for this usage.  Currently, Octave appears to treat no index as the equivalent of the magic colon ':' index.


tmp{} === tmp{:}


A reduced bit of code to reproduce the error message is


tmp = { zeros(0,1), zeros(0,1), 1 };
[tmp{}]



Rik <rik5>
Group administrator
Fri 19 Apr 2019 01:58:46 AM UTC, comment #3: 

Sounds like an issue with the implementation/semantics of cat() or its relatives, not the interpreter/parser.

The error message it gives here is weird: because you're doing horizontal concatenation here, it shouldn't care about the horizontal dimensions being consistent at all, just the vertical dimensions.

And even though the vertical dimensions are mismatched here, IIRC, empties are usually treated as a special case, and empties of any shape are treated as [], basically a no-op in terms of concatenation, like they are in your first `[...]` invocation.

Code note: you could do this more concisely by just passing the shape of empty you want to ones().


tmp{1} = 1;
tmp{2} = ones(0, 1);
tmp{3} = ones(0, 1);
tmp{4} = 1;


Andrew Janke <apjanke>
Fri 19 Apr 2019 01:52:30 AM UTC, comment #2: 

Compatibility note: is that `tmp{}` supposed to be valid Octave? Looks like it's acting as an alias for `tmp{:}`, but I didn't know that was a thing. Pretty sure Matlab doesn't support that.

Andrew Janke <apjanke>
Fri 19 Apr 2019 01:48:07 AM UTC, comment #1: 

title became a little wrong dosent matter if there is a number at the end the array or cell as long as there is a conflict between (1x1) and (0xN) N<1

SillyMon <sillymon>
Fri 19 Apr 2019 01:41:08 AM UTC, original submission:  

+verbatime+
tmp{1} = 1;
tmp{2} = ones(3)(logical(zeros(3)));
tmp{3} = ones(3)(logical(zeros(3)));
tmp{4} = 1;
[tmp{}]
tmp{1} = ones(3)(logical(zeros(3)));
[tmp{}]
-verbatime-

The last move of values in tmp to an array gives
error: horizontal dimensions mismatch (0x2 vs 1x1)

there is no problem with only one [](0x1).

I am not sure if this is interpreter material or something else

SillyMon <sillymon>

 

(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

Digest:
   bug dependencies.

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by apjanke (Posted a comment)
  • -email is unavailable- added by sillymon (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-04-23 rik5 Open/ClosedOpen Closed
    2019-04-23 rik5 StatusReady For Test Fixed
    2019-04-23 jwe StatusConfirmed Ready For Test
    2019-04-19 rik5 Dependencies- Depends on bugs #31615
    2019-04-19 rik5 Item GroupUnexpected Error or Warning Missed Error or Warning
        Summarynon-existent cell indexing within matrix brackets results in concatenation error non-existent cell indexing 'C{}' should produce an error
    2019-04-19 rik5 Severity3 - Normal 2 - Minor
        Priority5 - Normal 3 - Low
        StatusNone Confirmed
        Release4.4.1 dev
        Operating SystemMicrosoft Windows Any
        Summarypseudocode [ { [](0x1) [](0x1) [](0x1) 1 }{}, 1 ] fails, [ 1 ,{ [](0x1) [](0x1) 1 }{}, 1 ] works non-existent cell indexing within matrix brackets results in concatenation error

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code