bugGNU Octave - Bugs: bug #51333, Reductions on an empty matrix is...

 
 

bug #51333: Reductions on an empty matrix is inconsistent

Submitter:  Joel Dahne <urathai>
Submitted:  Wed 28 Jun 2017 01:21:12 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Confirmed Assigned to:  None
Originator Name:  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

Wed 01 Mar 2023 06:31:15 PM UTC, comment #12: 

@sarrah, can you comment on this code versus your bug #61690 patch? is it basically the same thing but applied to dot since that's a standalone function? could the same thing be applied to all of these other functions @Kai mentioned in comment #1?   can tests be added to this patch to ensure it solves the problem and no future regression?

(I checked and the patches still apply cleanly. I attached a patch file that has all the changes as a single patch)

(file #54432)

Nicholas Jankowski <nrjank>
Group Member
Wed 11 May 2022 02:20:23 PM UTC, comment #11: 

the two versions of your patch build on each other.  someone cannot apply the last version of your patch without having applied the first one. Note also since the two patches you uploaded are named the same thing, it may not be obvious that one depends on the other.

It's best when updating a patch to roll back to a clean state, then apply all of the changes so they are contained in a single patch file.  (sometimes this also needs to be done to 'refresh' patches in old bug reports that no longer apply to the current source anymore).

You do not need to reclone the source.  It may not be the best way (others more experienced can correct/add to my suggestions) but  you can 'rebase' a patch using 'hg strip' and 'hg patch' commands:

(This assumes you aren't using branches to make changes alonge another  developmnet line, and also the process will destroy any pending changes you've made, so don't do it if you have other changes not captured elsewhere, exported to a patch, etc.)

To get back to the current development tip (where you would be from  a fresh source clone) 'hg strip -r tip' will step you back one commit at a time. (Note it will also create backup bundles for recovery unless you use --no-backup, and that it will stop you if you have any uncommitted edits which you can override (carefully!) with a -f.)

depending on how many draft commits you have on your clone, at some point a summary will show commit: (clean) and update: (current), e.g,: 


$ hg summ
parent: 30992:4ef25c610433 tip
 format: Add 'default' format option (bug #62430)
branch: default
bookmarks: @
commit: (clean)
update: (current)


if you know how far back you have to go, you can specify the revision  to go back to in the strip command instead of '-r tip'.

from there, 'hg pull' and 'hg update' to be sure you have the latest revision. 

then, 'hg patch --no-commit <filename>' for the patches you want to apply (in this case, reapply your first then your second patch).  make any further changes you want, then do a single 'hg commit' to capture all of the changes, and 'hg export' a single patch others can use to test your changes.

generally, after exporting and uploading a patch, unless I'm testing further related changes I will repeat the 'hg strip' of my changes so that the next thing I'm doing will be based on a clean source. I can always reapply my own patch as above to make updates or changes. Branches can be a better way to manage multiple independent tasks as long as you're still generating 'clean' patches for others to test.

Nicholas Jankowski <nrjank>
Group Member
Wed 11 May 2022 01:14:49 PM UTC, comment #10: 

comment #9:

> Your patch dous not apply to current sources,
> it looks like it applies to a file that was already modified by yourself.
>
> Could you please provide a changeset with respect to the current development branch?


The source code I had worked on was obtained following the instructions here : https://wiki.octave.org/Building, i.e cloned from https://www.octave.org/hg/octave . When you say it doesn't apply to current sources, do you mean I should just pull the latest changes and commit my patch again or do I need to clone the development branch?

Sarrah Bastawala <sarrah>
Tue 10 May 2022 12:38:36 PM UTC, comment #9: 

Your patch dous not apply to current sources,
it looks like it applies to a file that was already modified by yourself.

Could you please provide a changeset with respect to the current development branch?

Carlo de Falco <cdf>
Group Member
Mon 18 Apr 2022 12:44:27 PM UTC, comment #8: 


>
> which although seem to have solved for empty matrices, shows slight differences when dimensions are specified.


I have understood the mistake in my understanding, and by changing the code after the reduction has taken place for the special case of an empty matrix, I was able to procure these results :
 

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


which are completely compatible with what Matlab does. I have added some tests to check for empty inputs, and my changeset passes all pre-existing and new tests. Please have a look!

(file #53105)

Sarrah Bastawala <sarrah>
Sun 17 Apr 2022 02:34:47 PM UTC, comment #7: 


As I worked on bug #61690, to make functions sum, prod etc that use function do_mx_red_op to perform reductions, and was able to succesfully make it consistent with Matlab, I tried to work and see if a similar logic would work to solve this as well.

Going through the comments,

comment #2:

> Yes, that's what I found as well. This means that if we would choose to go with Matlabs version it is just a matter of adding a check for "dim < 0" in the following if-statement.
>
> For "dot"  we add, right after the definition of dimx and dimy


this unfortunately is not possible or understandable to me as this code doesn't seem to exist in dot.cc file anymore. Instead, I worked on the function get_red_dims , and was able to procure these results :


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


which although seem to have solved for empty matrices, shows slight differences when dimensions are specified. This is because my code change looks like :


  if(x(1)==0 && dim == 0){ printf("here"); z(0) = 0;}
  else{
  for (int i = 0; i < nd; i++)
    {
      if (i < dim)
        {
          z(i) = x(i);
          tmp_m *= x(i);
        }


and when dim is specified as 1 by the user, it is converted to 0 for index conventions in vectors.

The only reason I am doing this is to make it compatible with Matlab, which seems to do this :


>> dot([],[])

ans =

     0

>> dot([],[],1)

ans =

  1×0 empty double row vector

>> dot([],[],2)

ans =

  0×1 empty double column vector


How exactly would we want to proceed? And if we want to make this reciprocate Matlab behaviour, does anyone have an idea of how we could differenciate between dot([],[]) and dot([],[],1), as dim=1 doesn't seem to work.

Sarrah Bastawala <sarrah>
Tue 20 Mar 2018 07:43:15 PM UTC, comment #6: 

As mentioned this needs to be implemented in the dot.cc file and cannot be done as a m-file, mainly due to performance reasons.

Apart from that I can also give you some feedback on your code. From what I can tell it has the required functionality, but there are things that can be improved in the code.

- Your error messages give error messages when called. The problem is you calling function message(), which does not exist. One example on how to do it can be found in this file [1]

- Take a look at the Octave style guide [2]. For example it is recommended to use double quoted strings and end blocks with a specific ending, for example endif for if-blocks.

If you want to try and modify the dot.cc file you have to be able to build Octave yourself. I don't know which operating system you are using, but some general info can be found here [3]. It can however be a bit complicated, especially if something goes wrong.

[1]: https://sourceforge.net/p/octave/interval/ci/default/tree/inst/@infsup/private/str2decimal.m
[2]: https://wiki.octave.org/Octave_style_guide
[3]: https://wiki.octave.org/Build_from_source

Joel Dahne <urathai>
Sun 18 Mar 2018 09:34:07 PM UTC, comment #5: 


> Quote SUDHIR KUMAR SUMAN
>
> Can't  we just remove that . cc file and replace with  it  i.e  . m file.


Please reply only via this bug-tracker;

   https://savannah.gnu.org/bugs/?51333

The email is just for notification, not for answering.

"dot" is a core function, that means it should be super fast, as it is often called.  M-files are slower in execution (they require interpretation).  Furthermore, this function should be offered to users that compile against libinterpreter (see standalone programs [1]).  Thus your solution is unfortunately no feasible solution.

Maybe you can try to solve this issue on C++ level, not just as a quick fix, but this requires some more time.

[1]: https://octave.org/doc/interpreter/Standalone-Programs.html#Standalone-Programs

Kai Torben Ohlhus <siko1056>
Group Member
Sun 18 Mar 2018 07:46:44 PM UTC, comment #4: 

Thank you for your interest and code submission.  I am sure your code works pretty well, but this bug requires a solution on C++ level in dot.cc.

https://hg.savannah.gnu.org/hgweb/octave/file/a73dedb0d9b5/libinterp/corefcn/dot.cc

Kai Torben Ohlhus <siko1056>
Group Member
Sat 17 Mar 2018 11:44:53 PM UTC, comment #3: 

I have gone through the bug #51333 and have implemented code for dot function. Similarly it can be implemented for others.I have also consider case when dimension is negative.
The link for the code is :
https://github.com/Alok009/Octave/blob/master/dot.m

Output from octave:

>> dot([],[])

ans = [](1x0)

Output after implimenting

>> dott([],[])

ans = 0

SUDHIR KUMAR SUMAN <sudhir27>
Fri 30 Jun 2017 07:33:38 AM UTC, comment #2: 

Yes, that's what I found as well. This means that if we would choose to go with Matlabs version it is just a matter of adding a check for "dim < 0" in the following if-statement.

For "dot"  we add, right after the definition of dimx and dimy,


// Inconsistency: dot ([], []) = 0
if (dimx.ndims () == 2 && dimx(0) == 0 && dimx(1) == 0 &&
    dimy.ndims () == 2 && dimy(0) == 0 && dimy(1) == 0)
  {
    dimx(1) = 1;
    dimy(1) = 1;
  }


if we go with the version Octave has now. If we choose the matlab version we also add "nargin == 2" to the condition.

I don't know if we should go with the Matlab version, but either way I think it makes sense to do the changes to "dot".

Joel Dahne <urathai>
Thu 29 Jun 2017 10:24:56 PM UTC, comment #1: 

Applies for the development version as well.

Digging into the code of "sum", "xsum", "prod", "sumsq" of an double NDArray, I found the inline function "do_mx_red_op" to be called in any case. According to a comment

https://hg.savannah.gnu.org/hgweb/octave/file/tip/liboctave/operators/mx-inlines.cc#l1537


// M*b inconsistency: sum ([]) = 0 etc.


it has to do with the described problem.

For "dot" the situation might be easier, as it is a seperate file which could check for empty input values:

https://hg.savannah.gnu.org/hgweb/octave/file/tip/libinterp/corefcn/dot.cc#l67

Kai Torben Ohlhus <siko1056>
Group Member
Wed 28 Jun 2017 01:21:12 PM UTC, original submission:  

Some of the reduction functions have inconsistent handling of empty matrices. They also differ slightly to how Matlab handles it. The function I have looked at are sum, sumsq, prod and dot but it might hold true for other reduction functions as well.

Both sum and prod have the convention that the empty matrix returns the unit, i.e.


> sum ([])
ans = 0
> sumsq ([])
ans = 0
> prod ([])
ans = 1


But this does not hold true for dot


> dot ([], [])
ans = [](1x0)


I think it would be more natural for dot to also return 0 here. This is also how Matlab does it


## In Matlab
> dot ([], [])
ans = 0


Further I noticed that Matlab and Octave handles the empty matrix differently when a dimension is specified. In Octave we have

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


Looking at the code for Octaves reduction function we in fact have a explicit conversion from [] to a matrix of size 1x0 (commented as an inconsistency). In Matlab we instead have


## In Matlab
> sum ([])
ans = 0
> sum ([], 1)
ans = [](1x0)
> sum ([], 2)
ans = [](0x1)


When no dimension is specified it handles the empty matrix in a special way. But when a dimension is specified it handles it like any other matrix.

Joel Dahne <urathai>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #54432:  bug51333_full.patch added by nrjank (1KiB - application/octet-stream - single patch combines file #53102 and 53105)
file #53105:  bug51333.patch added by sarrah (2KiB - text/x-patch - Added patch to resolve bug and make code compatible with Matlab.)
file #53102:  bug51333.patch added by sarrah (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by cdf (Posted a comment)
  • -email is unavailable- added by sarrah (Posted a comment)
  • -email is unavailable- added by sudhir27 (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by urathai (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-03-01 nrjank Operating SystemGNU/Linux Any
    2023-03-01 nrjank Attached File- Added bug51333_full.patch, #54432
    2022-04-18 sarrah Attached File- Added bug51333.patch, #53105
    2022-04-17 sarrah Attached File- Added bug51333.patch, #53102
    2017-06-29 siko1056 StatusNone Confirmed
        Release4.2.1 dev

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code