bugGNU Octave - Bugs: bug #33523, Matlab compatibility issue with...

 
 

bug #33523: Matlab compatibility issue with averaging the 3rd dim of a 2-D array

Submitter:  Arnaud Delorme <arnodelorme>
Submitted:  Sat 11 Jun 2011 03:24:05 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  lachlan
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 20 Feb 2016 05:28:23 AM UTC, comment #29: 

There isn't a way to do #include.  If you put commands in a file with extension .m, but don't have a function declaration, then the block of commands can be inserted directly into a file simply by using the name.  This is sort of like a #define.


file1.m:

function y = file1 (x)
  ...
  file2;
  ...
endfunction

file2.m:
cmd1;
cmd2;
cmd3;


I think perfect is the enemy of good in regards to this bug report.  Rather than craft the perfect solution, I have modified just the statistical functions which were what prompted this report.  I have done the minimum amount necessary to fix the problem which is simply removing a single input validation check on the maximum size of the input DIMENSION.  See this cset (http://hg.savannah.gnu.org/hgweb/octave/rev/7fbecef105ca).

I found some other problems along the way but I will report these as new bug reports.

Rik <rik5>
Group administrator
Tue 16 Feb 2016 10:12:20 PM UTC, comment #28: 

That's a good point, Rik.

Is there anything like a #define or #include for .m files?  I think that throw_if_not_natural is quite a common operation, and the code would be more maintainable if it was coded once.  If not, it should(?) be fairly easy to implement a directive "inline_source" that would act as #include, and could easily be ported to Matlab by defining the function "inline_source.m" to simply do a "source".

Lachlan Andrew <lachlan>
Mon 15 Feb 2016 08:41:06 PM UTC, comment #27: 

I think it would be better not to have a separate function to check the input argument.  Alas, calling out to a function does bring a fair amount of overhead and for small function bodies it doesn't make sense.

Rik <rik5>
Group administrator
Sun 14 Feb 2016 10:41:12 PM UTC, comment #26: 

Thanks, Philip.  I had forgotten to add  throw_if_not_natural.m  to modules.mk.  It works find from the .build directory :)

Here's a new patch.

(file #36338)

Lachlan Andrew <lachlan>
Mon 08 Feb 2016 06:57:20 PM UTC, comment #25: 

When testing the gui patch for bug #46795 (dbstop ....) I got several messages about "throw_if_not_natural". That function is created in the patch for this bug here.

I'm not sure how this relates to bug #46795 but it made me a bit wary that this fix and that of bug 46975 somehow interacted adversely.
But it could also have been due to another bug fix, as I had several other fixes to test in my source tree at the time.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 22 Jan 2016 10:41:13 PM UTC, comment #24: 

Oops.  The previous patch had some tabs in it :(  Please consider the new one of the same name.

(file #36131)

Lachlan Andrew <lachlan>
Fri 22 Jan 2016 10:13:36 AM UTC, comment #23: 

Rik, at long last here is the patch.

I was having a brain fade when I said that var and std should give NaN -- they obviously give 0.  It is kurtosis and skewness that are NaN (since they normalize by variance).

I've written a function "scripts/general/throw_if_not_natural.m" which reports an error if its argument is not a positive real scalar integer.  I expect there are other functions that could use it (for example, the previous statistics/base functions forgot to check the argument was real), but I didn't want to clutter this patch with those changes.

Thanks for all your patience in teaching me the coding standards, and revising and applying my patches.

(file #36130)

Lachlan Andrew <lachlan>
Fri 25 Sep 2015 09:21:01 PM UTC, comment #22: 

I just checked the original patch that jwe made for mean.m and it is very simple.  Although there are a lot of instances of this in the code I think we should concentrate on the ones that are generating Matlab incompatible results.  These functions are the ones in the scripts/statistics directory.

@Lachlan: If you prepare a cset for the functions in statistics that you think need this treatment I will apply it.



Rik <rik5>
Group administrator
Thu 17 Sep 2015 09:19:00 AM UTC, comment #21: 

Is there something stopping the supplied patch from being applied?


Also, in the list Rik posted, some of the functions should give NaN for a singleton dimension.  I suggest keeping the check for those cases.

The offenders are:
./statistics/base/var.m
./statistics/base/kurtosis.m
./statistics/base/std.m

I'm happy to go through these files and do the fix if will be applied...

Lachlan Andrew <lachlan>
Sun 21 Sep 2014 08:54:02 AM UTC, comment #20: 

Same Issue in #43038 with attached patch

Markus Bergholz <markuman>
Fri 03 Jan 2014 06:28:37 AM UTC, comment #19: 

According to comment #11, "A quick grep shows that the code snippet which checks dimensions is used 18 times in the statistics/base directory and 8 other places in the scripts directory. These would also need updating."

A current grep shows only 24 instances confined to these files


./signal/unwrap.m
./general/trapz.m
./general/shift.m
./general/prepad.m
./general/cumtrapz.m
./general/postpad.m
./statistics/base/run_count.m
./statistics/base/histc.m
./statistics/base/iqr.m
./statistics/base/zscore.m
./statistics/base/ranks.m
./statistics/base/skewness.m
./statistics/base/quantile.m
./statistics/base/var.m
./statistics/base/median.m
./statistics/base/kurtosis.m
./statistics/base/statistics.m
./statistics/base/center.m
./statistics/base/mode.m
./statistics/base/prctile.m
./statistics/base/meansq.m
./statistics/base/mean.m
./statistics/base/moment.m
./statistics/base/std.m


Rik <rik5>
Group administrator
Thu 02 Jan 2014 10:23:50 PM UTC, comment #18: 

Yes, I think this compatibility issue should be fixed.  It has not been done for the current 3.8.0 release.

Is this problem limited to the mean function or are others also affected?  If so, which ones?

John W. Eaton <jwe>
Group administrator
Mon 27 Aug 2012 06:04:17 PM UTC, comment #17: 

Hi,
I have come across the same issue using octave recently (which I just switched to). I completely agree with John's comment. In my example, I need to take the mean of the 3rd dimension, but only in some arbitrary range (1:2 for instance) which can happen to be singleton.

>> a = zeros(3,4,5); k=1:2;
>> mean(a(:,:,k),3)

ans =

        0        0        0        0
        0        0        0        0
        0        0        0        0

If the range appears to be singleton, it bugs:

>> k=1:1;
>> mean(a(:,:,k),3)

error: mean: DIM must be an integer and a valid dimension
error: called from:
error:   /usr/share/octave/3.4.2/m/statistics/base/mean.m at line 111, column 5

Strangely, I do not get a bug when doing the same thing on the second dimension:

>> mean(a(:,k,k),2)

ans =

        0
        0
        0

This is due to the second dimension singleton being not removed. It does not look coherent to me.

Is there any plan to get this corrected?

Fred <fffred>
Fri 29 Jun 2012 07:47:04 PM UTC, comment #16: 

John,

What is best for the programmer is usually right.  So, I
agree with your argument as an exception to the general
rule that augument checking should be done as precisely
as possible.

It might be good to mention this choice in the documentation.

Michael Godfrey <godfrey>
Group Member
Fri 29 Jun 2012 07:27:02 PM UTC, comment #15: 

You can certainly make a solid argument that stripping trailing singletons was a design mistake for Matlab. but given that behavior, I don't think it is reasonable to require that programmers write a lot of extra code to cope with it.  Should we also error for things like


x = rand (m, n, p);
x(:,:,1)


if p happens to be 1?  In that case ndims(x) == 2.  So should it also be an error to use more than two indices even if all the extra indices are equal to 1?

Should sum(x,3) also be an error in this case?  What about size(x,3)?  Both of these are currently valid in Matlab and Octave now.  So why shouldn't the mean function allow sum(x,3)/size(x,3) to work?

Unless you also want to make all these other operations produce errors, I don't see why it makes sense for the stats functions to produce errors for similar cases.

If you make all of those cases errors, you will significantly reduce Matlab compatibility.  I don't see that as a good thing to do.

John W. Eaton <jwe>
Group administrator
Fri 29 Jun 2012 07:09:02 PM UTC, comment #14: 

Regarding jwe's example, I think that a trailing singleton dimension is a special case and it would be better for the programmer to realize it and program for it.

I'm imagining a case where you might be adding 2-D data sets from various files into a 3-D array and then taking the mean along the 3rd dimension.  If there is only one file and only one 2-D data set (p=1) then the entire loop is unnecessary.  Octave's warning pushes back on the programmer and indicates that their assumptions were wrong to begin with.

The real use case is when someone types 'mean (x, 12)' on a vector or 2-D array.  The 12 is a typo, they may have meant 1 or 2 but both keys got hit.  Currently Octave issues a warning about this being out-of-bounds, but Matlab will return the original matrix.  Down the line, the script will probably error out because the dimensions of 'mean (x, 2)' are not the same as those of x; Only it will be harder to debug because it will be farther from the root cause of the problem.

Rik <rik5>
Group administrator
Fri 29 Jun 2012 06:33:27 PM UTC, comment #13: 

I agree with Rik.  Except, this is more than a "slight"
philosophical difference.  It is an important point of
principle.  I think that all functions should validate
their arguments based on the documented specification.
(It would be very nice if this could be automatied.)

It is a lot better for users to learn right away that they
have made an invalid call, rather than, as in this case,
that they assume that something went wrong in the function...

It is not appropriate to use Matlab compatibility as
an argument against this.

Michael Godfrey <godfrey>
Group Member
Fri 29 Jun 2012 06:08:29 PM UTC, comment #12: 

Using more indices than dimensions makes perfect sense given that Matlab strips trailing singleton dimensions.  So you might be forming x(n,m,p) and computing the mean along the third dimension.  But sometimes p == 1 and x appears to only have two dimensions.  I don't think that should be an error.

John W. Eaton <jwe>
Group administrator
Fri 29 Jun 2012 05:29:01 PM UTC, comment #11: 

There is a slight philosophical difference that has developed between Octave and Matlab.  Octave has generally been checking its inputs more rigorously in order to avoid Garbage In/Garbage Out calculations.  Matlab behaves a bit more like Perl in that it assumes the programmer is correct regardless of how strange the request may be.

Both approaches are valid, but it would be best to choose an overall approach rather than having some functions be lax and others strict.  My personal preference is for the stricter interpretation.  In this case, why doesn't the programmer know enough about their input data not to overstep the dimensions?  Shouldn't they be checking for that?  There is no reason to call mean on the non-existent dimension since it will just return the original matrix.

We are not slavish to Matlab compatibility where it looks unnecessary.  However, if we want to adopt Matlab's laissez-faire philosophy we should do so more generally.  A quick grep shows that the code snippet which checks dimensions is used 18 times in the statistics/base directory and 8 other places in the scripts directory.  These would also need updating.

Rik <rik5>
Group administrator
Fri 29 Jun 2012 04:03:55 PM UTC, comment #10: 

Looks right to me.

Michael Godfrey <godfrey>
Group Member
Fri 29 Jun 2012 04:03:30 PM UTC, comment #9: 

Oops, how about this change?  My previous attachment had some extraneous diffs included.


(file #26123)

John W. Eaton <jwe>
Group administrator
Fri 29 Jun 2012 03:59:57 PM UTC, comment #8: 

How about the attached patch?

(file #26122)

John W. Eaton <jwe>
Group administrator
Fri 29 Jun 2012 03:40:22 PM UTC, comment #7: 

This seems to be a reasonable request, and helps with
Matlab compatibility.  However, the behavior shown below
seems not to match the Matlab doc:

>> help mean

 MEAN   Average or mean value.
    For vectors, MEAN(X) is the mean value of the elements in X. For
    matrices, MEAN(X) is a row vector containing the mean value of
    each column.  For N-D arrays, MEAN(X) is the mean value of the
    elements along the first non-singleton dimension of X.
 
    MEAN(X,DIM) takes the mean along the dimension DIM of X.
 
    Example: If X = [0 1 2
                     3 4 5]
 
    then mean(X,1) is [1.5 2.5 3.5] and mean(X,2) is [1
                                                      4]
 
    Class support for input X:
       float: double, single
=====================

Given this, what is the correct behavior for mean(X,3)?
Matlab says:

>> x = [ 0 1 2; 3 4 5]


x =

     0     1     2
     3     4     5

>> mean(x,2)


ans =

     1
     4

>> mean(x,3)


ans =

     0     1     2
     3     4     5

>> mean(x,4)


ans =

     0     1     2
     3     4     5

>

Should the documentation or the behavior be implemented?


Michael Godfrey <godfrey>
Group Member
Fri 29 Jun 2012 03:31:52 PM UTC, comment #6: 

This actually is a pretty easy fix. Do you want to attempt it yourself?

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Fri 29 Jun 2012 02:59:17 PM UTC, comment #5: 

I sure wish this would be reopened.  mean() is an important function and we would like to be able to use Octave again someday for our large codebase.

Anonymous
Wed 27 Jul 2011 10:58:57 PM UTC, comment #4: 

This is with Octave 3.4.0 by the way.

Arnaud Delorme <arnodelorme>
Wed 27 Jul 2011 10:54:25 PM UTC, comment #3: 

Under Octave

x = mean (magic(3), 3)
error: mean: DIM must be an integer and a valid dimension
error: called from:
error:   /Volumes/donnees/Applications/Octave.app/Contents/Resources/share/octave/3.4.0/m/statistics/base/mean.m at line 111, column 5

>> y = mean (eye(2), 4)

error: mean: DIM must be an integer and a valid dimension
error: called from:
error:   /Volumes/donnees/Applications/Octave.app/Contents/Resources/share/octave/3.4.0/m/statistics/base/mean.m at line 111, column 5

Under Matlab

x = mean (magic(3), 3)

x =

     8     1     6
     3     5     7
     4     9     2

x = mean (magic(3), 4)

x =

     8     1     6
     3     5     7
     4     9     2

y = mean (eye(2), 4)

y =

     1     0
     0     1



Arnaud Delorme <arnodelorme>
Wed 27 Jul 2011 10:43:12 PM UTC, comment #2: 

No response to request for information in 1 month.  Closing report.  This may be re-opened.

Rik <rik5>
Group administrator
Mon 13 Jun 2011 03:33:58 AM UTC, comment #1: 

When you say it works fine, what is the output?  Presumably it should be the matrix P itself.  Also, how does Matlab handle dimensions above 3?

Sample code to run:


x = mean (magic(3), 3)
y = mean (eye(2), 4)


Rik <rik5>
Group administrator
Sat 11 Jun 2011 03:24:05 PM UTC, original submission:  

P = rand(2,2); 
Pori = mean(P, 3); 
crashes if P is only 2 dim (works fine under Matlab)

Arnaud Delorme <arnodelorme>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #36338:  bug_33523_v2.cset added by lachlan (29KiB - application/octet-stream)
file #36131:  bug_33523.cset added by lachlan (29KiB - application/octet-stream)
file #36130:  bug_33523.cset added by lachlan (29KiB - application/octet-stream)
file #26123:  diffs.txt added by jwe (600B - text/plain)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by mtmiller (Updated the item)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by markuman (Posted a comment)
  • -email is unavailable- added by fffred (Posted a comment)
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by godfrey (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by arnodelorme (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 20 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-02-20 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2016-02-14 lachlan Attached File- Added bug_33523_v2.cset, #36338
    2016-02-08 lachlan Assigned toNone lachlan
    2016-02-08 lachlan CategoryInterpreter Octave Function
        Item GroupIncorrect Result Matlab Compatibility
        StatusConfirmed Ready For Test
    2016-01-22 lachlan Attached File- Added bug_33523.cset, #36131
    2016-01-22 lachlan Attached File- Added bug_33523.cset, #36130
    2016-01-18 mtmiller Release3.6.2 dev
    2014-12-27 rik5 Dependencies- bugs #43038 is dependent
    2012-06-29 jwe Attached File- Added diffs.txt, #26123
    2012-06-29 jwe Attached File#26122 Removed
    2012-06-29 jwe Attached File- Added diffs.txt, #26122
    2012-06-29 jordigh StatusNeed Info Confirmed
        Open/ClosedClosed Open
        Release3.4.0 3.6.2
        Operating SystemMac OS Any
    2011-07-27 rik5 Open/ClosedOpen Closed
    2011-06-13 rik5 StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code