bugGNU Octave - Bugs: bug #63460, mean should support operation on...

 
 

bug #63460: mean should support operation on more than 2 remaining dimensions for Matlab compatibility

Submitter:  Rik <rik5>
Submitted:  Sat 03 Dec 2022 05:46:51 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  9.1.0 Planned Release:  9.1.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 02 Mar 2023 05:36:02 AM UTC, comment #19: 

I verified Octave now returns the correct answer for the motivating code example.  Marking as Fixed and closing report.

Rik <rik5>
Group administrator
Wed 01 Mar 2023 03:08:12 PM UTC, comment #18: 

patch #10301 has been pushed to default that includes a fix for this bug in mean (and ensures the same behavior in median, std, var). marking as ready for test with a dependence on that patch.

Nicholas Jankowski <nrjank>
Group Member
Sun 26 Feb 2023 10:27:34 PM UTC, comment #17: 

after release i'll create a patch over at patch #10314 for the four functions after adjusting header and any other format changes for octave core. any other specific issues can be tracked there.

Nicholas Jankowski <nrjank>
Group Member
Sun 26 Feb 2023 08:30:04 PM UTC, comment #16: 

Yes, all 4 functions are (more or less*) fully compatible. Perhaps, we could leave some time after releasing statistics-1.5.4 so that we can track any discrepancies in behavior or bugs after it becomes accessible to a wider audience of octave users.

*there are some extra features regarding the weights in var and std

Andreas Bertsatos <pr0m1th3as>
Sun 26 Feb 2023 07:26:17 PM UTC, comment #15: 

So, after 2/28/2023 mean.m can be ported to core.  In addition, it sounds like median.m, var.m, and std.m could be ported.

Rik <rik5>
Group administrator
Fri 24 Feb 2023 11:26:23 PM UTC, comment #14: 

For everyone's information, the statistics-1.5.4 release has been pushed forward and it is scheduled for 28.2.2023.

Andreas Bertsatos <pr0m1th3as>
Fri 24 Feb 2023 04:58:13 AM UTC, comment #13: 

Yes, that work has taken care of almost all outstanding bugs related to mean. Until it’s ported over to core though the bug is still present. I created a patch report where I’ll track bringing them in, then I’ll go around cleaning up all of these reports. I think there’s a 1.5.4 release coming soon, I might wait for that to hit so it establishes a specific version I’m bringing over, rather than plucking them off the statistics development branch.

Nicholas Jankowski <nrjank>
Group Member
Thu 23 Feb 2023 11:18:55 PM UTC, comment #12: 

Checking in on this bug report. I recall there was much activity in the last couple of months by nrjank and pr0m1th3as in the statistics package and the default branch. Is this report still relevant after those fixes?

Arun Giridhar <arungiridhar>
Group Member
Wed 07 Dec 2022 12:13:59 PM UTC, comment #11: 

I just pushed a new commit of the `mean` function in the statistics package at GitHub, which has adapted Rik's code snippet to handle properly the vecdim option without the for loops and the N-3 dim restriction of my previous implementation. The current implementation works identically to MATLAB, but any further testing is very welcome, because I might still be missing something

The respective implementations for `median`, `var`, and `std` functions will be available there by the 15th of December. Hopefully, a core octave developer can review them and push them to octave 8.

Andreas Bertsatos <pr0m1th3as>
Tue 06 Dec 2022 09:54:21 PM UTC, comment #10: 

@jwe - regarding your comment #2 other functions:

there are at least half a dozen separate reports on the lack of "all" support and vecdim supports for different functions.  When the feature was released in 2018b, the list included:
all, min, any, mode, bounds, prod, max, std, mean, sum, median, var. 

in 2019 the release notes indicated you could now use a vecdim in many other functions without specifying them.

There are a similar set of separate bug reports for "all". 

I believe there have been some implementations of each, and likely some pending patches attached to reports.

i think at one point we had suggested collecting all such reports and creating a single 'support new dim options in many functions' bug report. This was sort of done with a retitle for bug #61098, but all of the other reports are still sitting out there.

would consolidating those be a worthwhile exercise?

Nicholas Jankowski <nrjank>
Group Member
Sun 04 Dec 2022 01:19:37 AM UTC, comment #9: 

I think there is an easier way to solve the problem of supporting a vector of dimensions.  The key is to understand that all of the dimensions selected by VECDIM will be operated on and reduced to a single value.  So, what we want to do is place all of the data that belongs to the selected dimensions into a single dimension DCALC and then call whatever function we want (say sum()) and specify that it operate over that one DCALC dimension.  This can be done pretty easily in Octave by using permute/reshape/ipermute.  Sample code is shown below and attached as m-file do_vecdim.m.  There are two important aspects of this solution: 1) it is general and works on any number of specified dimensions, 2) it only uses built-in, i.e., compiled functions so it is quite fast compared to a for loop solution.


function y = do_vecdim (hfcn, x, vecdim)

  ## Input validation
  ## Remove dimensions larger than actual array
  vecdim(vecdim > ndims (x)) = [];
  if (isempty (vecdim))
    y = x;
    return;
  endif

  ## Calculate permutation vector and permute
  remdims = 1:ndims (x);    # all dimensions
  remdims(vecdim) = [];     # delete ones specified by vecdim to leave remaining
  perm = [remdims, vecdim];

  y = permute (x, perm);

  ## Reshape to put all vecdims in final dimension
  szy = size (y);
  nremd = numel (remdims);
  sznew = [szy(1:nremd), prod(szy(nremd+1 : end))];
  y = reshape (y, sznew);

  ## Call function handle on single, squashed dimension
  y = hfcn (y, nremd+1);

  ## Inverse permute back to correct dimensions
  y = ipermute (y, perm);

endfunction


To use the function try something like


sz = [2, 2, 3, 4];
N = prod (sz);
x = reshape (1:N, sz);

vecdim = [1,3];

do_vecdim (@sum, x, vecdim)




(file #54052)

Rik <rik5>
Group administrator
Sat 03 Dec 2022 06:41:13 PM UTC, comment #8: 

I 've recently pushed a new commit on the `mean` function in the statistics package so that the `mean` function can handle up to 3 dimensional pages and 10 dimensions for data input, including error checks that were missing.

I understand that the nested for loops are a problem in terms of speed when implemented through m files, but at least we have the missing functionality. My intention is to make the implementations for the functions that are missing the functionality for vecdim, nanflag, etc, and hopefully re-implement in c++ later on.

If it's not possible to include these in the forthcoming octave 8 release, I would suggest that they will find their way into later releases, but at least they will become available at some point.

Andreas Bertsatos <pr0m1th3as>
Sat 03 Dec 2022 06:09:17 PM UTC, comment #7: 

I agree with jwe.  It's too late to put big functionality changes into the 8.1 release.

Rik <rik5>
Group administrator
Sat 03 Dec 2022 04:23:18 PM UTC, comment #6: 

I'm not sure that any significant changes to functions are appropriate at this point in the release process.

John W. Eaton <jwe>
Group administrator
Sat 03 Dec 2022 04:21:01 PM UTC, comment #5: 

There are some changes incoming in the next 10-12 days for median, std, var, and possibly sum, min, and max. https://octave.discourse.group/t/new-maintainer-for-the-statistics-package/2791/97

Arun Giridhar <arungiridhar>
Group Member
Sat 03 Dec 2022 02:20:55 PM UTC, comment #4: 

Prior to supporting multiple dimensions, I would say that this was a feature request.  Now that mean() supports multiple dimensions, but not as well as Matlab, I would call it Matlab Compatibility.  Alternatively, you could argue that Octave supports multiple dimensions up to 2 and therefore it is a feature request to support more than 2 dimensions.

This is a specific hack in mean.m.  Long term, the Array class probably needs updating to support "omitnan"/"includenan" and also to support a vector of dimensions for operations like sum().

Rik <rik5>
Group administrator
Sat 03 Dec 2022 06:46:11 AM UTC, comment #3: 
Anonymous
Sat 03 Dec 2022 06:16:48 AM UTC, comment #2: 

Maybe I misunderstood?  I see now those dimensions are beyond the size of the array.

Is there already another feature request for handling array-valued DIM arguments for functions like sum, prod, all, etc?

John W. Eaton <jwe>
Group administrator
Sat 03 Dec 2022 06:09:20 AM UTC, comment #1: 

I'd say that's a feature request to have DIM specified as an array.  If I remember correctly, functions like mean originally only worked on columns.  Then Matlab added a DIM argument to select another dimension.  But only one was only allowed.  Then recently they changed again to allow arrays of dimensions.

Yes, we have incompatible behavior, but the reason is because we are chasing new features, not because we did something else that just happened to be different but reasonable.

John W. Eaton <jwe>
Group administrator
Sat 03 Dec 2022 05:46:51 AM UTC, original submission:  

The following test code works on Matlab, but throws an error on Octave:


x = reshape (1:72, 3,3,4,2);
mean (x, [1,7,9])


This shouldn't be a problem.

A separate issue, but one that might get addressed in re-working the algorithm, is that there are for loops which are going to be slow in m-files.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #54052:  do_vecdim.m added by rik5 (774B - text/x-matlab)

 

Digest:
   patch dependencies.

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 pr0m1th3as (Posted a comment)
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (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-02 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.1.0
    2023-03-01 nrjank StatusConfirmed Ready For Test
        Dependencies- Depends on patch #10314
    2022-12-04 rik5 Attached File- Added do_vecdim.m, #54052

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code