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
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

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