patchGNU Octave - Patches: patch #10314, matlab compatible std, var, mean,...

 
 

patch #10314: matlab compatible std, var, mean, median to core from statistics package

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Tue 14 Feb 2023 10:24:53 PM UTC
   
 
Category:  Core : new feature Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  nrjank Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 02 Mar 2023 11:42:12 PM UTC, comment #7: 

I think we're good and this patch report could be closed as Done.

Rik <rik5>
Group administrator
Thu 02 Mar 2023 10:09:35 PM UTC, comment #6: 

I don't think there's a problem with high-dimensional data as long as you are using a 64-bit octave_idx_type which just about everyone is these days.  If you want to use 32-bit types on a Raspberry Pi you can do so, but that's not really a platform for high performance numerical computing.

The reason is that the number of elements on the squashed dimension is


sz = size (data);
nel = prod (sz(squashed_dimensions));


In order to overflow the octave_idx_type which is int64_t the number of elements would have to exceed 2^63 - 1.  If this was a double array then each element would take 8 bytes and this would be 2^66 bytes.  I don't know any machine that has that type of memory.  Hence, there would have been an out-of-memory error when the attempt was made to create the array well before any squashing could happen.

Rik <rik5>
Group administrator
Thu 02 Mar 2023 07:40:35 PM UTC, comment #5: 

2) since we can't get the details from the parser on the penalty from if(flag) (that all gets lumped into the functions 'self' time i think), i saw explicitly the time used for calling false, which should be from the flag=false lines. I didn't explicitly check to see if the time reduction was on the same order or not, as it was small enough it may have been tough to tease out over normal variability.  but it was enough that I kept it in. For now I guess we cough it up to m-code function calling overhead. small enough though that code readability taking precedence makes sense.

(it also never occurred to me that all class names are functions and feval was much cleaner)

in median the issue wasn't so much in calling sum for dimensions other than 1, it was trying to vectorize selecting different pair of numbers to sum in each column. I didn't see a way to do that with sub2ind with an unknown number of dimensions unless my working direction was dim1 and i could flatten reference all following dimensions with :, making it work for any number of dimensions.  I'm not a huge fan of that flattening though, as noted in the FIXME we could accidentally run into octave_index_type limits for high dimensionality data.

Nicholas Jankowski <nrjank>
Group Member
Thu 02 Mar 2023 06:28:17 PM UTC, comment #4: 

On question 1, I found it easier to understand the reshape process if the dimensions were at the end.  When indexing, you can use the magic colon ':' to collapse all remaining dimensions and this is what the code reminded me of.  For example,


x = reshape (1:24, 2,3,4)
x =

ans(:,:,1) =

   1   3   5
   2   4   6

ans(:,:,2) =

    7    9   11
    8   10   12

ans(:,:,3) =

   13   15   17
   14   16   18

ans(:,:,4) =

   19   21   23
   20   22   24

# Now preserve index 1 (rows) and collapse dimensions 2 and 3 into final dimension (2 = columns)
x([1 2], :)
ans =

    1    3    5    7    9   11   13   15   17   19   21   23
    2    4    6    8   10   12   14   16   18   20   22   24

# Equivalently, use reshape
reshape (x, 2, 3*4)
ans =

    1    3    5    7    9   11   13   15   17   19   21   23
    2    4    6    8   10   12   14   16   18   20   22   24


When the dimensions are not at the end the reshape vector looks a bit awkward.  From median, the code is


      ## Reshape all vecdims into dim1
      num_dim = prod (szx(dim));
      szx(dim) = [];
      szx = [num_dim, ones(1, numel(dim)-1), szx];
      x = reshape (x, szx);


I find the need to insert a vector of ones awkward.  But, it isn't a super big deal and that's why I left it alone in median because it looked like it was going to involve a fair number of changes to calls to sum() etc. where the dimension would no longer be 1 but a variable 'dim'.  I'm okay, though, if we want to rationalize this.

On question 2, that's weird that there is a performance penalty for using true/false.  Was the performance penalty in the assignment 'flag = true' or was it in the evaluation of 'if (flag)'?  I can kind of see that 'flag = true' might be slower because true() is a function, whereas a number like 0 or 1 is directly understood by the parser.  Still, I'm surprised it makes that much of a difference.

Rik <rik5>
Group administrator
Thu 02 Mar 2023 04:31:40 PM UTC, comment #3: 

Thanks for the cleanup. I really had thought I scrubbed them. (Even the 1 line dim checks. I really thought I had done that but must have reverted/not restored it somewhere along the way)

2 questions -
1) is there a particular reason to permute the operating dimensions to the end rather than the front? performance? preference?   in median (where i see you left it at the front) it was done that way to ease the flattened omitnan processing with sub2ind, etc., and that works much better for nD arrays when you can count on the primary dimension being in 1 and whatever's left  following. For the other functions i just followed suit.

2) funny enough I had switched all the logical flags from t/f to 1/0 when running tic/tocs and the profiler and saw that calls to true/false were putting a ~3-5% penalty on runtime. I thought it would be the opposite, with a conversion to logical happening inside ifs. It's hardly significant compared to all the other input validation cruft, but had no idea why it would work that way. 

any other streamlining advice on these welcome.  the extra input handling did put a hit on performance and some of the codepaths, especially vecdim handling, aren't exactly speedy.

Nicholas Jankowski <nrjank>
Group Member
Thu 02 Mar 2023 05:29:07 AM UTC, comment #2: 

I checked in a changeset that updates the code to conform to best practices for Octave core.  See http://hg.savannah.gnu.org/hgweb/octave/rev/6a2638cbea96.

Rik <rik5>
Group administrator
Wed 01 Mar 2023 06:16:05 AM UTC, comment #1: 

statistics 1.5.4 just released with matlab compatible and fairly well debugged versions of mean/median/std/var.

attached are versions of those functions with headers changed to match Octave core. Have also rewritten / cleaned up a some of the docstring text. Unlike in the statistics pkg, std is left as a wrapper to sqrt(var), so only the header/help text is changed to reflect the new options/behavior inherited from var.

functions contain a number of expanded BISTs from the previous core and statistics versions, as well as addressing a number of other compatibility bugs.  (a noted exception to this is the recently identified bug #63848 on sum overflows giving bad answers for mean), and a build/test shows now other core function test failures with the change.

pushed the revised functions & NEWS changes to default as
https://hg.savannah.gnu.org/hgweb/octave/rev/c6eeb8b44c28
and marking ready for test.

(file #54424, file #54425, file #54426, file #54427)

Nicholas Jankowski <nrjank>
Group Member
Tue 14 Feb 2023 10:24:53 PM UTC, original submission:  

There are a number of separate bug reports that address pieces of bringing the functions named above up to full matlab compatibility.  primary concerns are:
- proper empty input handling
- compatible vector dimension handling
- "all" dimension option handling
- nanflag handling (option to ignore nan values)
- compatible outtype handling

The statistics package has shadowed these functions for octave core and very recently all four functions were made fully matlab compatible.  mean had been ported from statistics to core previously, but compatibility was still incomplete and there was forked development.

I've worked with the statistics package to bring the functions into a compatible state, including avoiding any mean regressions, and the plan is now to merge those changes back into core octave.  Moving forward then statistics can just incorporate any changes to core and shadow them for older versions of octave (targeting v9.1)

In addition, these versions of mean and median should solve bug #54567 regarding large int overflow and double precision limits.

The functions need some header and other text rework to match core function style.  Once I do that cleanup I'll attach those versions to this report and push a patch for review after i verify they don't seriously break any calling functions in core. I'll also try to go through the pile of related bug reports and update/close/dependency-mark them accordingly.

Note: i've done some optimization and speed testing, but the extra input handling for the functions does add overhead. some advice on refactoring to buy back some of that performance would be appreciated.


Nicholas Jankowski <nrjank>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #54424:  mean.m added by nrjank (20KiB - text/plain - functions versions ported from statistics 1.5.4 into core targeting v9.1)
file #54425:  var.m added by nrjank (30KiB - text/plain - functions versions ported from statistics 1.5.4 into core targeting v9.1)
file #54426:  median.m added by nrjank (23KiB - text/plain - functions versions ported from statistics 1.5.4 into core targeting v9.1)
file #54427:  std.m added by nrjank (5KiB - text/plain - functions versions ported from statistics 1.5.4 into core targeting v9.1)

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (Submitted the item)
  • -email is unavailable- added by nrjank
  •  

    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 logged-in users can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-03-03 nrjank StatusReady For Test Done
        Open/ClosedOpen Closed
    2023-03-01 nrjank Dependencies- bugs #63460 is dependent
    2023-03-01 nrjank Attached File- Added mean.m, #54424
        Attached File- Added var.m, #54425
        Attached File- Added median.m, #54426
        Attached File- Added std.m, #54427
        StatusIn Progress Ready For Test
    2023-02-28 nrjank Dependencies- bugs #54567 is dependent
    2023-02-14 nrjank Carbon-Copy- Added pr0m1th3as

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code