bugGNU Octave - Bugs: bug #61690, Behavior of sum() with empty inputs

 
 

bug #61690: Behavior of sum() with empty inputs

Submitter:  Guillaume <gyom>
Submitted:  Fri 17 Dec 2021 02:23:58 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  None Assigned to:  None
Originator Name:  guillaume Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 02 Mar 2023 11:58:24 AM UTC, comment #18: 

comment #17:

> patch still applies cleanly.  can we add self tests to this to verify that it gets sum to behave appropriately for empty inputs and prevent future rejections?


Yes, so according to the changes I had made, I had added more tests in addition with the existing ones to make sure the changes are as desired. These are implemented for the `cumsum, cumprod, sum and prod` functions in the `libinterp/corefcn/data.cc` file.
 

> @sarrah, can you comment on how this does or doesn't relate to bug #51333?  sum is mentioned there, and you have a patch sitting there for dot.  is this report just duplicative of one of the bugs mentioned there?


According to my understanding, it's not exactly a duplicate. Afaik, the expected output i.e the way it should react to empty inputs/outputs and the underlying logic is same.

However, the changes to be made are in two complete different files and hence the two different bugs seem correct to me.

Also, in accordance to how @siko1056 has mentioned in https://savannah.gnu.org/bugs/index.php#comment1 , the main change in source code in my patch too has been done in the `liboctave/operators/mx-inlines.cc` file, where the main function `do_mx_red_op` is called in the functions whose tests now show the expected change.

Sarrah Bastawala <sarrah>
Wed 01 Mar 2023 06:27:24 PM UTC, comment #17: 

patch still applies cleanly.  can we add self tests to this to verify that it gets sum to behave appropriately for empty inputs and prevent future rejections?

the code does appear to follow the current behavior of sum and many other functions where all input functions, including empties, have outputs with the operating dimension changed to size=1, except for 0x0 inputs. those output as 1x1. effectively, it changes dim1 to 1 then operates on default first non-singleton dim 2 (changing the 1x0 input -> 1x1 output). 

@sarrah, can you comment on how this does or doesn't relate to bug #51333?  sum is mentioned there, and you have a patch sitting there for dot.  is this report just duplicative of one of the bugs mentioned there?

Nicholas Jankowski <nrjank>
Group Member
Wed 22 Jun 2022 12:57:00 PM UTC, comment #16: 

The changeset now does apply cleanly.

Apart from fixing the coding style, could you please
explain better what the cangeset does and why?

The relevant part of the code appears to be this


--- a/liboctave/operators/mx-inlines.cc        Tue May 31 23:34:23 2022 -0400
+++ b/liboctave/operators/mx-inlines.cc        Wed Jun 01 18:59:30 2022 +0530
@@ -1542,8 +1542,15 @@
   octave_idx_type l, n, u;
   dim_vector dims = src.dims ();
   // M*b inconsistency: sum ([]) = 0 etc.
-  if (dims.ndims () == 2 && dims(0) == 0 && dims(1) == 0)
-    dims(1) = 1;
+  if(dims.zero_by_zero () )
+  {
+    if(dim!=-1)
+    {
+      dims(dim) = 1;
+    }
+    else
+      dims(1) = 1;
+  }


can you comment on it a bit?

BTW the correct formatting would be :


if (dims.zero_by_zero ())
  {
    if (dim != -1)
      dims(dim) = 1;
    else
      dims(1) = 1;
}


Carlo de Falco <cdf>
Group Member
Thu 02 Jun 2022 11:31:11 AM UTC, comment #15: 


comment #14:

>
> Your patch does not apply to current sources.
> Could you please provide a changeset with respect to the current development branch?


My understanding of the mercurial workflow was slightly poor, I have tried to make a patch applicable to current sources and attached it. It would be great if you could have a look at it.

Sarrah Bastawala <sarrah>
Tue 10 May 2022 02:29:09 PM UTC, comment #14: 


comment #12:

>
> comment #11:
> > Here are some examples of how my change fixes the bug :
>
> Apologies for my carelessness, but I just noticed that after running the tests I had mistakenly made a typo in the data.cc file that would eventually result in a build failure. I have fixed the commit and am uploading the revised patch of the amended commit here. Please have a look!
>
> (file #53095)


Your patch does not apply to current sources.
Could you please provide a changeset with respect to the current development branch?

Carlo de Falco <cdf>
Group Member
Mon 09 May 2022 01:33:44 AM UTC, comment #13: 

To try all behaviors of sum() out, we may design a complete test suite, which should contain:

Argin | FunctionReturn | MATLAB/Octave
scalar |...

Every incompatibility of sum() of one MATLAB version should belong to one bug number

Glite <linuxbckp>
Sat 16 Apr 2022 04:12:07 PM UTC, comment #12: 


comment #11:

> Here are some examples of how my change fixes the bug :


Apologies for my carelessness, but I just noticed that after running the tests I had mistakenly made a typo in the data.cc file that would eventually result in a build failure. I have fixed the commit and am uploading the revised patch of the amended commit here. Please have a look!

(file #53095)

Sarrah Bastawala <sarrah>
Wed 13 Apr 2022 10:17:25 AM UTC, comment #11: 

comment #10:

> it's worth checking that the tests in those other functions do still match matlab output.


I have now crosschecked all provided tests in the data.cc file for relevant functions such as cumprod, cumsum, prod and sum, and also added some extra tests that work against both Octave and Matlab. The only function now remaining slightly incompatible with Matlab is dot, and I believe that should be addressed in bug #51333 as the changes would have to be made to a seperate file.

Here are some examples of how my change fixes the bug :

octave:1> sum([])
ans = 0
octave:2> sum([],1)
ans = [](1x0)
octave:3> sum([],2)
ans = [](0x1)
octave:4> prod([])
ans = 1
octave:5> prod([],1)
ans = [](1x0)
octave:6> prod([],2)
ans = [](0x1)
octave:7> sum(ones(3,0),2)
ans =

   0
   0
   0

octave:8> sum(ones(1,0),1)
ans = [](1x0)
octave:9> sum(ones(3,0,4,2))
ans = [](1x0x4x2)


All of these are consistent with Matlab.

Sarrah Bastawala <sarrah>
Tue 05 Apr 2022 09:10:15 PM UTC, comment #10: 

sorry for the confusion.  I meant that before figuring out how to adapt your change to avoid breaking those other tests, it's worth checking that the tests in those other functions do still match matlab output. I think I looked through some statistics functions (mean, median, etc.) 5 years ago where there were 'compatibility' tests that no longer matched what matlab does, as it had changed its handling of some nan and empty inputs.

If I had time I was going to peek at those tests and check them out, but haven't gotten there yet today.

Nicholas Jankowski <nrjank>
Group Member
Tue 05 Apr 2022 07:03:36 PM UTC, comment #9: 


comment #8:

> OK. It might be tedious, but it would be worth verifying those other tests against MATLAB to be sure they match current behavior.


I am slightly confused, did you mean run the existing tests in data.cc that pass with my fix in place against Matlab to make sure? However, this fix does pose a problem for empty inputs in functions like prod, dot, etc

Sarrah Bastawala <sarrah>
Tue 05 Apr 2022 11:57:25 AM UTC, comment #8: 

OK. It might be tedious, but it would be worth verifying those other tests against MATLAB to be sure they match current behavior.

Nicholas Jankowski <nrjank>
Group Member
Tue 05 Apr 2022 11:45:42 AM UTC, comment #7: 

comment #4:

> i'm not set up to compile c-code, so haven't checked your

patch, but looking at sum, and having played with some other
functions that have odd outputs from different empty inputs,
the following are also expected by matlab (2021b):

All of these gave me the correct output on my fix, maybe it was that they already worked.

> I see now that sum already has a number of empty input tests in /libinterp/corefcn/data.cc, but strangely none for a [] input. so you should verify that your fix doesn't break any of those tests, and add BISTs to data.cc that capture this fix.) 

I did verify that all tests passed in data.cc using make check before submitting the patch, so exiting tests should not be an issue. If we do decide to go ahead with the expected output for this fix, I will add the required BISTs.

>
> and do any other functions than sum rely on this function in mx-inlines.cc? would want to make sure it doesn't change or break any other functions while trying to fix sum.


However this does seem to be a problem. Most of the functions like prod, dot, sumsq, that use the do_mx_red_op function in mx-inlines.cc, are mentioned in bug #51333, and which fail by return empty vectors where they should be returning unit sums for some testcases provided in the bug's comments. So while my fix would work for all edge cases in sum, it wouldnt be the right logic to apply for some other functions. I am confused as to whether this can possibly be resolved by code logic, or if the problem lies in how we are re-using the matrix reduction function.

>
> for commit messages, use imperative tense, not past tense (tell what happens when i apply the patch, not what you did in creating it.) So "Resolve" not "Resolved",  Extend" not "Extended", etc.


I will change this from the next commit onwards.

Sarrah Bastawala <sarrah>
Tue 05 Apr 2022 05:21:57 AM UTC, comment #6: 

Most of mx-inlines.cc was written a long time ago.  At some point, a "zero_by_zero" function was added to the dim_vector class that appears to be the same as the check


dims.ndims () == 2 && dims(0) == 0 && dims(1) == 0


It also seems bad to repeat most of the exact same test twice.

So maybe write


if (dims.zero_by_zero ())
  {
     if (dim == 0)
       dims(0) = 1;
     else if (dim == 1)  // OR just "else" if we are certain
       dims(1) = 1;      // that DIM can be only 0 or 1 here.
  }


John W. Eaton <jwe>
Group administrator
Tue 05 Apr 2022 12:52:52 AM UTC, comment #5: 

also I just noticed bug #51333, which this seems to be a duplicate of. Should review the discussion over there and see what is/isn't applicable to your fix.

Nicholas Jankowski <nrjank>
Group Member
Tue 05 Apr 2022 12:26:38 AM UTC, comment #4: 

i'm not set up to compile c-code, so haven't checked your patch, but looking at sum, and having played with some other functions that have odd outputs from different empty inputs, the following are also expected by matlab (2021b):


>> sum([],1)
ans =
  1×0 empty double row vector

>> sum([],2)
ans =
  0×1 empty double column vector

>> sum(ones(0,0),1)
ans =
  1×0 empty double row vector

>> sum(ones(0,0),2)
ans =
  0×1 empty double column vector

>> sum(ones(1,0),2)
ans =
     0

>> sum(ones(3,0),2)
ans =
     0
     0
     0

>> sum(ones(1,0),1)
ans =
  1×0 empty double row vector

>> sum(ones(3,0),1)
ans =
  1×0 empty double row vector

>> sum(ones(1,0),2)
ans =
     0

>> sum(ones(3,0),2)
ans =
     0
     0
     0

>> sum(ones(3,0),3)
ans =
  3×0 empty double matrix

>> sum(ones(3,0,4,2),3)
ans =
  3×0×1×2 empty double array


what it appears to be doing is pre-casting the output with something like


size_output = size(input);
size_output(dim) = 1;
output = zeros(size_output);


how does your fix address or change sum for inputs like those?  (i didn't test them against Octave, so maybe they already work? I see now that sum already has a number of empty input tests in /libinterp/corefcn/data.cc, but strangely none for a [] input. so you should verify that your fix doesn't break any of those tests, and add BISTs to data.cc that capture this fix.)

and do any other functions than sum rely on this function in mx-inlines.cc? would want to make sure it doesn't change or break any other functions while trying to fix sum.

for commit messages, use imperative tense, not past tense (tell what happens when i apply the patch, not what you did in creating it.) So "Resolve" not "Resolved",  Extend" not "Extended", etc.

Nicholas Jankowski <nrjank>
Group Member
Mon 04 Apr 2022 10:08:06 PM UTC, comment #3: 

I have submitted a patch that resolves the bug upto my knowledge. Here are the results after the fix:

octave:3> sum([],1)
ans = [](1x0)
octave:4> sum([],2)
ans = [](0x1)


which is clearly compatible with Matlab that shows the following results:

>> sum([],1)

ans =

  1×0 empty double row vector

>> sum([],2)

ans =

  0×1 empty double column vector


Hopefully a maintainer or mentor sees this bug and approves of the change soon!

Sarrah Bastawala <sarrah>
Mon 04 Apr 2022 05:53:42 PM UTC, comment #2: 

we generally aren't very rigorous with assigning bugs to individuals, and I think Savannah only allows assignments to project members.  For now, unless anyone comments otherwise, consider it yours to work on as there haven't been any comments since the bug was filed.

Consistent empty array handling is something that appears in many basic functions, as much octave code fails to differentiate between [], [](0x1), [](1x0), etc., yet alone produce outputs in a matlab compatible fashion. I'd be curious if the change to mx-inlines.cc would improve other functions as well. we should also be prepared for a change to possibly affect tests for other functions, assuming they have tests to detect such changes.

Nicholas Jankowski <nrjank>
Group Member
Mon 04 Apr 2022 05:31:45 PM UTC, comment #1: 

Hello, I am working on resolving this bug and would like it to be assigned to me. I saw that while

sum([],1)

 behaves differently on Octave and Matlab,


sum([],2)

 gives,

ans =

  0×1 empty double column vector

on Matlab, and


ans = [](0x1)

on Octave, which are similar enough. And by playing with how we set default dimensions when we encounter an empty matrix in liboctave/operators/mx-inlines.cc, I believe the bug can be resolved to give a similar output for dim = 1 as well.

Sarrah Bastawala <sarrah>
Fri 17 Dec 2021 02:23:58 PM UTC, original submission:  

There is a difference between Octave and Matlab with sum() when the first input is empty and a dimension is provided:

Octave:


octave:1> sum([])
ans = 0
octave:2> sum([],1)
ans = 0


Matlab:


>> sum([])
ans =
     0
>> sum([],1)
ans =
  1x0 empty double row vector


Guillaume <gyom>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #53269:  bug61690_a.patch added by sarrah (3KiB - text/x-patch - Added patch that can be applied and resolved earlier errors.)
file #53095:  bug61690.patch added by sarrah (3KiB - text/x-patch - Amended mistake in commit for patch to resolve bug and make code compatible to Matlab)
file #53083:  bug61690.patch added by sarrah (3KiB - text/x-patch - Added patch to resolve bug and make code compatible with Matlab.)
file #53044:  bug61690.patch added by sarrah (1KiB - text/x-patch - Added patch to resolve bug and make code compatible with Matlab.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by cdf (Posted a comment)
  • -email is unavailable- added by linuxbckp (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by sarrah (Posted a comment)
  • -email is unavailable- added by gyom (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-06-02 sarrah Attached File- Added bug61690_a.patch, #53269
    2022-04-16 sarrah Attached File- Added bug61690.patch, #53095
    2022-04-13 sarrah Attached File- Added bug61690.patch, #53083
    2022-04-04 sarrah Attached File- Added bug61690.patch, #53044

    Back to the top

    Powered by Savane 3.13-aa77.
    Corresponding source code