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

 
 

bug #51333: Reductions on an empty matrix is inconsistent

Submitted by:  Joel Dahne <urathai>
Submitted on:  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:  GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

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>
Project 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>
Project 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,

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

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

But this does not hold true for dot

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

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

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

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:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-06-29 siko1056 StatusNone => Confirmed
        Release4.2.1 => dev

    Back to the top


    Powered by Savane 3.3