bugGNU Octave - Bugs: bug #55622, for loop does not run over a...

 
 

bug #55622: for loop does not run over a matrix with colums but no row

Submitter:  Marco Caliari <caliari>
Submitted:  Fri 01 Feb 2019 03:02:30 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  10.1.0 (current default) Planned Release:  10.1.0 (current default)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 22 Feb 2024 12:29:15 AM UTC, comment #33: 

Closing report as Philip as verified the fix.

Rik <rik5>
Group administrator
Wed 21 Feb 2024 10:43:15 PM UTC, comment #32: 

Thanks

Philip Nienhuis <philipnienhuis>
Group Member
Wed 21 Feb 2024 09:02:20 PM UTC, comment #31: 
John W. Eaton <jwe>
Group administrator
Wed 21 Feb 2024 08:13:50 PM UTC, comment #30: 

... and cc'íng Petter

Philip Nienhuis <philipnienhuis>
Group Member
Wed 21 Feb 2024 08:12:09 PM UTC, comment #29: 

The patch from comment #27 fixes the regressions I get when running the test suite with bytecode-interpreter enabled.

Can it be pushed, please?

Philip Nienhuis <philipnienhuis>
Group Member
Sat 17 Feb 2024 08:56:19 AM UTC, comment #28: 

I think the fix for the bytecode-interpreter is an easy one. I attach a patch for it.

(file #55712)

Fernando <tutissanalio>
Fri 16 Feb 2024 08:23:37 AM UTC, comment #27: 

This should be addressed also in the bytecode-interpreter branch. Currently, changes in the tests have introduced some regressions in that branch. Maybe Petter is not aware of this bug report.


octave:3> addpath /home/fernando/tmp/octave-hg/test/
octave:4> test for.tst
***** test <*55622>
 cnt = 0;
 for k = zeros (0,3)
   cnt++;
 endfor
 assert (cnt, 3);
 assert (k, zeros (0,1));
!!!!! regression: https://octave.org/testfailure/?55622
ASSERT errors for:  assert (cnt,3)

  Location  |  Observed  |  Expected  |  Reason
     ()           0            3         Abs err 3 exceeds tol 0 by 3
***** test <*50893>
 cnt = 0;
 for k = cell (0,3)
   cnt++;
 endfor
 assert (cnt, 3);
 assert (k, cell (0,1));
!!!!! regression: https://octave.org/testfailure/?50893
ASSERT errors for:  assert (cnt,3)

  Location  |  Observed  |  Expected  |  Reason
     ()           0            3         Abs err 3 exceeds tol 0 by 3
***** test <*55622>
 cnt = 0;
 for k = zeros (1,2,0);
   cnt++;
 endfor
 assert (cnt, 0);
 assert (k, zeros (1,0));
!!!!! regression: https://octave.org/testfailure/?55622
ASSERT errors for:  assert (k,zeros (1, 0))

  Location  |  Observed  |  Expected  |  Reason
     .         O(1x2x0)      E(1x0)      Dimensions don't match
warning: test: file /home/fernando/tmp/octave-hg/test/for.tst leaked global variables: __assert_printf__
PASSES 19 out of 22 tests


Fernando <tutissanalio>
Mon 12 Feb 2024 04:45:57 PM UTC, comment #26: 

No opposition in the last few days. So, I pushed jwe's patch to the default branch:
https://hg.savannah.gnu.org/hgweb/octave/rev/38112a5ee9c6

Markus Mützel <mmuetzel>
Group administrator
Thu 08 Feb 2024 04:34:09 PM UTC, comment #25: 

I agree. Fixing this so that Octave is compatible to Matlab in this respect would avoid future bug reports.
And we don't need to add a bunch of special cases to be compatible. We actually can remove one check (albeit an easy one).

I rebased jwe's patch from 2019 to a current head (unsurprisingly with a couple of conflicts). It still passes the tests.

If no one opposes, I'd like to push it to the default branch.


(file #55676)

Markus Mützel <mmuetzel>
Group administrator
Tue 30 Jan 2024 09:14:01 PM UTC, comment #24: 

It seems to me that the fix is simple (just remove "nrows > 0" from the loop test) and it is the same one I came up with again today when this problem was reported again almost exactly 5 years later (as bug #65228).

Or, is there some other case we need to consider - maybe just removing that test breaks something else?  I don't remember now.

It makes sense to me to fix Octave to do the compatible thing here since we know what that is, it does make some sense, and if we don't fix it, we will just periodically get bug reports about it users will get the impression that we are dumb for not making Octave to the Right Thing (and for these users, the Right Thing is always whatever it is that Matlab does).

Maybe these reports will only come in every 5 years or so, but still, how much time was wasted on this bug report again today?

John W. Eaton <jwe>
Group administrator
Tue 30 Jan 2024 07:40:52 PM UTC, comment #23: 

updating that this behavior still persists unchanged in both programs:

Matlab 2023b:

>> for i = (3:2)', disp ('hey'), end
hey
>> for i = zeros (0,3), disp ('hey'), end
hey
hey
hey
>> for i = zeros (0, 3, 0), i, end
>> for i = ones (0,3), disp ('hey'), end
hey
hey
hey
>>  for i = ones (3,0), disp ('hey'), end


Octave 9.0.1:

>> for i = (3:2)', disp ('hey'), end
>> for i = zeros (0,3), disp ('hey'), end
>> for i = zeros (0, 3, 0), i, end
>> for i = ones (0,3), disp ('hey'), end
>> for i = ones (3,0), disp ('hey'), end
>>


after all of the back and forth here and on previous reports, should this be moved to a won't fix, or let it sit pending future changes?  worth refreshing jwe's patch vs current codebase?

Nicholas Jankowski <nrjank>
Group Member
Mon 30 Nov 2020 04:14:17 AM UTC, comment #22: 

Whoops, sorry for the typo. Now I'm understand what is asked by the original post. It means that octave for loop will only iterate `column` by `column`, no matter about the dimensions.

I should understand it more clearly from the doc:


If var is a column vector or a matrix, var will be a column vector each time the loop body is executed.


Even if the matrix is in 3d shape, octave still loop over it column by column (not 2d matrix for each loop). Than i will stick in using looping a range and use that as the index.

Thank you and sorry.

Muhammad Yasirroni <yasirroni>
Mon 30 Nov 2020 03:28:40 AM UTC, comment #21: 

Regarding comment #20.  Please fix your confusion with a cup of coffee ☕🙂

Your code cannot be run, as there is a confusion between "y" and "loop_var".  Clear your workspace "clear all" and start again.


loop_var = ones(3,1)
isempty(loop_var)
size(loop_var)
for var = loop_var, disp('hey'), disp(var), end


Second all this discussion is about "ones(0,3)", NOT "ones(3,1)" or alike.  As Rik said, please read again carefully.  Your code of course works nicely with a single loop iteration in both Octave and Matlab, as the column count is "1".

No bugfixes by Matlab R2019a and R2020b yet.


>> for i = (3:2)', disp ('hey'), end
hey
>> for i = zeros (0,3), disp ('hey'), end
hey
hey
hey
>> for i = zeros (0, 3, 0), i, end
>> for i = ones (0,3), disp ('hey'), end
hey
hey
hey
>> for i = ones (3,0), disp ('hey'), end


I favor Riks opinion on this item too and suggest to close it as won't fix.

Kai Torben Ohlhus <siko1056>
Group Member
Mon 30 Nov 2020 02:38:57 AM UTC, comment #20: 

But, ones(3,1) is not empty? Sorry for the confusion,

And ordinary users who are not familiar with octave will most likely think of it as `loop using index`.


loop_var = ones(3,1)
isempty(y)
size(y)
for var = loop_var
    disp('hey')
    disp(var)

loop_var =

   1
   1
   1

ans = 0
ans =

   3   1

hey
   1
   1
   1


Muhammad Yasirroni <yasirroni>
Mon 30 Nov 2020 02:12:11 AM UTC, comment #19: 

"I don't understand why we debate over an empty matrix. Clearly, the column vector is not empty at all"

If you don't understand then re-read the discussion thread.  The issues are subtle and changing Octave's core looping behavior should not be done casually.

Clearly, the column vector is empty because both Matlab and Octave say so.


loop_var = ones (0, 3);
isempty (loop_var)
ans = 1



Rik <rik5>
Group administrator
Sun 29 Nov 2020 11:34:53 PM UTC, comment #18: 

I don't understand why we debate over an empty matrix. Clearly, the column vector is not empty at all. So, we should iterate over it.
Furthermore, if we got it as a result of a function, we sometimes didn't check the dimension of our matrix, such as:


b=[3;1;2];
[sorted_b, indices] = sort(b);
size(indices)
for a=indices
    disp('loop')
    disp(b(a))
end

ans =

   3   1

loop
   1
   2
   3


When we expect that we are able to iterate over indices.

Muhammad Yasirroni <yasirroni>
Sat 16 Feb 2019 11:16:21 AM UTC, comment #17: 

Or what if the compatible behavior is prohibited in the --safe mode?

Anonymous
Sat 16 Feb 2019 09:09:28 AM UTC, comment #16: 

What do you think if we leave the compatible behavior in the --traditional mode?

Marco Caliari <caliari>
Group Member
Fri 15 Feb 2019 10:24:12 PM UTC, comment #15: 

Quoting from the Matlab documentation on empty arrays


An empty array in MATLAB is an array with at least one dimension length equal to zero. Empty arrays are useful for representing the concept of "nothing" programmatically. For example, suppose you want to find all elements of a vector that are less than 0, but there are none. The find function returns an empty vector of indices, indicating that it couldn't find any elements less than 0.

A = [1 2 3 4];
ind = find(A<0)

ind =

  1x0 empty double row vector

Many algorithms contain function calls that can return empty arrays. It is often useful to allow empty arrays to flow through these algorithms as function arguments instead of handling them as a special case. If you do need to customize empty array handling, you can check for them using the isempty function.


The use of empty arrays, per se, is fine.  As The Mathworks point out, it is often useful to be able to write an algorithm without having to special case whether there will be any elements to act on.

As an example, consider transforming between 1-based indexing (which Octave uses) to 0-based indexing (which the C language uses).  This is pretty easy, you just need to subtract 1.


A = [1 2 3 4];
ind = find (A)
ind =

   1   2   3   4

new_ind = ind - 1
new_ind =

   0   1   2   3


And this algorithm still works well if we put in a condition


A = [1 2 3 4];
ind = find (A > 2)
ind =

   3   4

new_ind = ind - 1
new_ind =

   2   3


And finally, it still works when there are no elements


A = [1 2 3 4];
ind = find (A > 4)
ind = [](1x0)
new_ind = ind - 1
new_ind = [](1x0)


I don't see any problem with allowing empty matrices to participate in mathematical operations.  However, confusing code-dependent issues arise if you say that looping over empty matrices is permissible.

For example, a common coding strategy might be to calculate some collection of elements and then loop over them like so.


for elem = collection
  ...
endfor


For the command find(), Matlab returns the empty dimension in the second dimension (columns).  But there is nothing that guarantees that an empty matrix has 0 columns.  User code that defines the variable collection might choose the opposite convention and create 0xN style empty matrices in which case the for loop is operating N times.

Or something innocuous like a transpose could upset things.

Say I have an algorithm for which I want to operate on the rows of the matrix.  Since the for loop returns columns, I need to transpose the matrix first.


x = [1, 2; 3, 4];
for elem = x'
  elem
end
elem =

   1
   2

elem =

   3
   4


So far that's fine.  But if x is an empty matrix then the control becomes confusingly caught up with the decision to act on rows.  In the case of find() the 1x0 result becomes 0x1 and the loop is executed once rather than zero times.  It's also data dependent.


x = [1 2 3 4];
idx = find (x > 4)
idx = [](1x0)
idx = find (x' > 4)
idx = [](0x1)


Lastly, this is very likely to lead to syntax errors.  Most of the time, a programmer is going to want to do something with the loop element from the collection and this will require an indexing operation that fails because the element is empty.

Last example,


x = [1 2 3 4]';
idx = find (x > 4)
for var = idx
  var(1) = var(1) + 1;  % increment first element in variable
end


Under Matlab this errors out with "Index exceeds the number of array elements (0)".  Currently, Octave has no problem with this code because it doesn't execute the body of the for loop when the right-hand side is empty.


Rik <rik5>
Group administrator
Thu 14 Feb 2019 08:12:38 PM UTC, comment #14: 

It seems that MATLAB has used the concept of nullable type.
From the documentation:

"An empty array in MATLAB is an array with at least one dimension length equal
to zero. Empty arrays are useful for representing the concept of "nothing" programmatically."

All nonabstract MATLAB classes have a method named empty. For example we can write:


A = int16.empty(5,0);


It means that all non abstract types all nullable. Therefore we can make array of nulls and iterate over nulls.
--Interesting discussion--

Anonymous
Thu 14 Feb 2019 05:14:03 PM UTC, comment #13: 

I still disagree, and think we should leave Octave's current behavior as is.

In terms of UX/UI design I prefer to have the minimum number of principles and be able to to derive the behavior from these, rather than having lots of ad hoc exceptions.

For example, from the Declaration of Independence, "All men are created equal".  But, starting in 1896 (Plessy vs. Ferguson and doctrine of "separate but equal"), you had to special case the general principle based on location.  If you were in the North or West of the U.S. the general principle applied, but if you were in the South then you used the exception.  This turned out to be a dumb idea and the U.S eventually went back to just one guiding principle in 1954 (Brown vs. Board of Education of Topeka).

My conception of a for loop over an object (as opposed to a range like 1:1000) is that it is equivalent to a foreach loop over the elements of the object.  Hence, if the object is empty, there should be no looping.  For me, looping over empty matrices means creating a mental exception only for "for" loops which seems unneccessary.

Of course, Matlab, like Moses, has said, "looping occurs over the number of columns".  But, I guess I'm skeptical of the stone tablets.  We don't always follow Matlab when they veer off the rails which I think is the case here.

As a practical matter, I think user's code would be improved by choosing the loop variable more explicitly, rather than relying on this questionable behavior.  If you want to look a certain number of times, do that.


%% Clear code
for i = 1:5
  disp ("Hello World");
end

%% Unclear code
idx = zeros (0,5);
for i = idx
  disp ("Hello World");
end


Also as a practical matter, I don't think this odd construct occurs with much frequency so changing the few instances that occur wouldn't be burdensome.

With all that said, this isn't an issue I'm particularly interested in arguing over.  If you need to commit the patch for some clients who require 100% compatibility then it happens.

Rik <rik5>
Group administrator
Mon 11 Feb 2019 04:17:38 PM UTC, comment #12: 

I disagree.

The for loop in Matlab is documented to loop over the columns of a matrix.  When indexing an N-d array as a matrix, Matlab joins the columns from all dimensions as if they were one.  Octave also does this re-dimensioning when evaluating an N-d array in a for loop command.  The behavior is only different when the leading dimension is the only zero dimension.

For example, if you have an array like x = ones (2,3,4), you can write x(:,1), x(:,2), ..., x(:,12) to index all the columns.  This behavior is consistent for zero-sized arrays.  If you have x = ones (0,3,4), you can perform the same indexing.  Only when one or more of the dimensions beyond the first is zero do you have no columns to index in the re-dimensioned array.  By not looping when only the first dimension equal to zero, we are less compatible with Matlab and inconsistent with other array indexing.

We've already copied the weirder part of for N-d array for loops, which is to always assign the re-dimensioned value to the loop index even when there are zero trips through the loop.

I believe the attached change makes the behavior compatible.

(file #46234)

John W. Eaton <jwe>
Group administrator
Mon 11 Feb 2019 07:17:01 AM UTC, comment #11: 

I agree with the earlier decision and upholding it, closing.

Mike Miller <mtmiller>
Group Member
Mon 11 Feb 2019 01:03:51 AM UTC, comment #10: 

We decided not to implement this questionable behavior back in 2017.  Is there new evidence that would make us want to reverse that decision?  This is very much a corner case, and very odd.  My vote is to leave the decision as is and close this report.

Rik <rik5>
Group administrator
Sat 02 Feb 2019 07:32:02 PM UTC, comment #9: 

Again, that looks like an artifact of implementation.  How would one guess that in loops, N-D arrays are squeezed to 2-D before using as the looping structure.  All very unintuitive, hence a bad UX/UI, even if an underlying principle can be found.

Rik <rik5>
Group administrator
Sat 02 Feb 2019 01:39:59 PM UTC, comment #8: 

Rik:  It is consistent behavior if you first reshape the N-d array in


for i = ndarray
  ...
end


into a matrix using ndarray(:,:).

John W. Eaton <jwe>
Group administrator
Fri 01 Feb 2019 09:23:39 PM UTC, comment #7: 

@jwe: Emphasizing the irrationality of the behavior, the example you gave executes 0 times rather than 3.

Rik <rik5>
Group administrator
Fri 01 Feb 2019 09:15:39 PM UTC, comment #6: 

I had forgotten about the previous bug report but just found it now.  There, I wrote, "These seem almost like some kind of accident of implementation rather than conscious decisions about how things should work. But who knows, right? Is this Matlab behavior documented anywhere?"  I suppose it is documented, in that for loops are documented to iterate over the columns of a matrix.

BTW, what does the following do in Matlab?


for i = zeros (0, 3, 0)
  i
end


Does it loop three times or not at all?

John W. Eaton <jwe>
Group administrator
Fri 01 Feb 2019 07:36:12 PM UTC, comment #5: 

@Mike: good digging.  I'm inclined not to emulate Matlab.  Adding jwe to the CC list.

Rik <rik5>
Group administrator
Fri 01 Feb 2019 06:42:12 PM UTC, comment #4: 

For reference, this was last discussed in bug #50893. At that time, it was decided to be intentionally incompatible, the evaluator was changed to not loop over any empty array expression (https://hg.savannah.gnu.org/hgweb/octave/rev/fd7a16594614).

Mike Miller <mtmiller>
Group Member
Fri 01 Feb 2019 06:34:04 PM UTC, comment #3: 

I think it's debatable how useful this is.  It seems to be an artifact of the implementation: Matlab loops over the number of columns regardless of whether the actual expression is empty. 

For reference, this loop executes three times in Matlab


for i = zeros (0,3), disp ('hey'), end



Rik <rik5>
Group administrator
Fri 01 Feb 2019 03:59:43 PM UTC, comment #2: 

@Kai: I discussed it with jwe on IRC, I think he is fixing it.

Marco Caliari <caliari>
Group Member
Fri 01 Feb 2019 03:14:58 PM UTC, comment #1: 

Evil statement ;-)  And it loops even in Matlab R2018b, one can see 'hey' once there, while Octave does not loop.

Kai Torben Ohlhus <siko1056>
Group Member
Fri 01 Feb 2019 03:02:30 PM UTC, original submission:  

The following code


for i = (3:2)', disp ('hey'), end


does not loop, while it should since (3:2)' has one column. It used to loop at least up to 4.2.2.

Marco Caliari <caliari>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #46234:  loop-iteration-diffs.txt added by jwe (2KiB - text/plain)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by tutissanalio (Posted a comment)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by yasirroni (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by caliari (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 18 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-02-22 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2024-02-21 philipnienhuis Carbon-Copy- Added petter
    2024-02-17 tutissanalio Attached File- Added bug55622-bytecode.patch, #55712
    2024-02-12 mmuetzel Item GroupRegression Matlab Compatibility
        Fixed ReleaseNone 10.1.0 (current default)
    2024-02-12 mmuetzel StatusPatch Submitted Ready For Test
    2024-02-08 mmuetzel Attached File- Added bug55622-loop-iteration-diffs-v2.patch, #55676
        Planned ReleaseNone 10.1.0 (current default)
    2024-01-30 nrjank Dependencies- bugs #65228 is dependent
    2024-01-30 nrjank Operating SystemGNU/Linux Any
    2019-02-11 jwe Attached File- Added loop-iteration-diffs.txt, #46234
        StatusWont Fix Patch Submitted
        Open/ClosedClosed Open
    2019-02-11 mtmiller StatusConfirmed Wont Fix
        Open/ClosedOpen Closed
    2019-02-01 rik5 Carbon-Copy- Added jwe
    2019-02-01 siko1056 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code