bugGNU Octave - Bugs: bug #48690, basic statistics functions should...

 
 

bug #48690: basic statistics functions should return NaN on empty input

Submitted by:  None
Submitted on:  Wed 03 Aug 2016 03:08:24 PM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: Patch SubmittedAssigned to: None
Originator Name: Shelby SavageOriginator Email: -unavailable-
Open/Closed: OpenRelease: dev
Operating System: Any

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 Jump to the original submission)

Wed 26 Jul 2017 08:01:34 PM UTC, comment #33:

ok. recent discussion doesn't apply to cov, so most recent version of that along with the standalone cov patch should be fine.

realized my last mode fix was based on a not-the-latest patch and was missing some code cleanup. better version attached.

Thanks to a HDD crash, I'm currently working without my normal hg setup, so these m-files will have to do for now. If no one else wants to do it I'll get around to making a new patch collecting all of these when I get things set up again.

Also, worth noting that only cov has the 'nanflag' option handled (see bug #50007 and bug #50571) . Most of the others have at least that incompatibility still present.

additionally at least mean has other options different from the matlab version.

(file #41348)

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 07:52:16 PM UTC, comment #32:

updated std and var attached

(file #41346, file #41347)

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 07:19:04 PM UTC, comment #31:

updated median attached.

(file #41344)

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 07:02:00 PM UTC, comment #30:

updated mean attached. similar simple fixes worked, tests added.

(file #41343)

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 06:16:12 PM UTC, comment #29:

ok. passes 118 out of 118 tests :)

fully(?) matlab compatible mode.m that works with mode(X,DIM) form with empty inputs attached.

basically made what was the sz = [0,0] codepath only execute for nargin==1. for nargin ==2 let it change the specified dimension to 1 instead of the 'first nonsingleton dimension'. the rest follows. I'll attach an updated patch when I get a chance.

should be able to make similar changes to the other statistics functions.

(file #41342)

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 06:01:16 PM UTC, comment #28:

note, a number of the tests appended to comment #24 have the wrong form for c. once i fix that, i think this may be fixed using your &quot;point 3&quot; in comment #23

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 05:34:45 PM UTC, comment #27:

and i just now went back and realized what you were writing was all for mean not mode. I think what you wrote works for mean. I'll double check, generate tests to make sure its consistent, etc. maybe only mode will need these special cases.

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 05:17:27 PM UTC, comment #26:

here's the difference (in c) I have to capture:

matlab 2017a output:

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 05:09:12 PM UTC, comment #25:

apparently the new Preview button changes all > to &amp;gt;, and then subsequent submittal keeps them that way. sorry for the mess. anyone wanna file a savannah bug?

Nicholas Jankowski <nrjank>
Wed 26 Jul 2017 05:07:48 PM UTC, comment #24:

just want to confirm that yes, I don't have any tests checking the mode(X,DIM) codepath, and it does fail there.

Matlab 2017a:

Octave 4.2.1 (release)

in my modified mode, it short circuits to NaN without checking dim:

the rest of your summary of the empty nD matrix handling is generally correct. for all cases other than 0x0 the first nonsingleton dimension is changed to 1 and the rest are left alone.

The code does currently change the fist non-singleton dim (not just the first dim greater than 1) to 1 with:

and no need to use resize, since the output is just based on sz, not x. just change sz(2) = 1. then it can move through the &quot;not 0x0&quot; codepath as you said.

This, however, does create issues the dimensions of f and c. looking into that now. I've appended a number of additional test based on matlab 2017a output in addition to the ones that were already in there:

Nicholas Jankowski <nrjank>
Thu 20 Jul 2017 08:19:51 AM UTC, comment #23:

I think this is related to bug #51333. All of the functions you are refering to here can be considered reductions I think.

From my testing I seen the following rules regarding the dimension from Matlab.

1. If no specific dimension is specified and the input is not 0x0: set the first non-singleton dimension to 1 and keep the rest.
This is for example consistent with your example:

2. If no dimension is specified and the input is 0x0: Treat the input as size 0x1 (which means it returs NaN in your cases)
In Matlab

3. If a dimension is specified: Always set that dimension to one and keep the rest. NOTE that this means that 0x0 is not handled separately

In the last example it, as usual, chops trailing singleton dimensions

So I think the only things that needs to be changed is that if no dimension is given it should set the dimension to the first non-singelton dimension (not the first &amp;gt;1 as it currently is) and also if the input is 0x0 change it to 0x1 (doing this with resize should respect the input type right?).

Joel Dahne <urathai>
Mon 30 Jan 2017 02:41:30 PM UTC, comment #22:

sorry, forgot to add for request 2, that section starts at line 254

Nicholas Jankowski <nrjank>
Mon 30 Jan 2017 02:39:43 PM UTC, comment #21:

also, two requests if anyone wants to take a look at cov:

1 - I would appreciate if anyone had a good example of how using inputParser would have made varargin handling easier. Having little familiarity with it, I thought this would be a good example to learn on. But it seemed that by the time I was done writing all the validation functions, it wasn't going to be any less messy than the manual parsing I came up with, so I left it that way.

2 - the case of 'partialrows' for a single input is a bit odd. I wound up using a recursive call to cov with the 'omitrows' parameter in a double for loop. This does the job, but I'm sure it's ridiculously slow between the looping and the overhead from calling cov. If it's not to onerous, I'd appreciate suggestions on how to flatten it.

Nicholas Jankowski <nrjank>
Sun 29 Jan 2017 03:52:25 AM UTC, comment #20:

After a bit of discussion, I made a fully matlab compatible cov. Passes a bunch of tests against Matlab 2016b, includes updated doc section. This also includes an implementation for the nanflag option that was missing previously.

(file #39597, file #39598)

Nicholas Jankowski <nrjank>
Wed 18 Jan 2017 07:02:35 PM UTC, comment #19:

in the meantime a roll-up of all functions except for cov in a single patch are attached with some code style-fixes.

(file #39502)

Nicholas Jankowski <nrjank>
Wed 18 Jan 2017 04:10:23 PM UTC, comment #18:

hold off on the patch for cov. All others should be fine.

I missed its usage with multiple inputs, and that collides with previous accepted matlab incompatibility for the function. in short, if with numbers matlab would produce a matrix output, and octave's does not, should the 'matlab compatibility fix' for empty and nan inputs follow the matlab or octave convention?

ex, normal inputs:

ex, matlab outputs for this case:

also, there are a couple tests/errors in the current implementation that are problematic:

scalar 'y' treated only as an 'opt' for nargin==2 if it's ascalar, out of bounds error if it's not 0 or 1. Matlab doesn't error, but output is a matrix:

last, there's a special codepath at line 97 (scalar 'x' outputs 0) that shortcuts everything else, including code that should error out:

previous mailing list discussions on the accepted incompatibility:
http://octave.1599824.n4.nabble.com/covariance-matrix-td1607376.html
http://octave.1599824.n4.nabble.com/cov-x-y-matrix-or-number-td4465087.html

started a discussion on maintainers. no discussion but linking so nothing gets lost:
http://octave.1599824.n4.nabble.com/compatibility-of-cov-past-discussions-and-how-to-interpret-for-current-bugfix-td4681490.html

I've also cc'd a couple people involved in the past cov compatibility discussion.

Nicholas Jankowski <nrjank>
Tue 16 Aug 2016 03:42:07 PM UTC, comment #17:

ok, here's the final(?) patch for mode.

modeNaN_v2.diff

At this point everything covered by this bug has a patch addressing it, ready for review. Should be able to update bug status.

(file #38232)

Nicholas Jankowski <nrjank>
Mon 15 Aug 2016 08:21:06 PM UTC, comment #16:

here's everything but mode. multiple outputs are... odd.

latest versions for the others:

medianNaN_v3.diff
meanNaN_v3.diff
varNaN_v2.diff
stdNaN_v1.diff
covNaN_v2.diff (no change from one previously attached)

(file #38221, file #38222, file #38223, file #38224)

Nicholas Jankowski <nrjank>
Mon 15 Aug 2016 07:59:52 PM UTC, comment #15:

figured out a working pattern. relatively simple, passes all tests. I still suspect this was a hack without considering odd empty array cases, and later versions of matlab might change the behavior. left TODO notes in there to check against future versions.

will be uploading final (?) patches shortly.

Nicholas Jankowski <nrjank>
Mon 08 Aug 2016 05:46:53 PM UTC, comment #14:

bad post. fixing:

was going to upload last patch for std([]) = NaN when I saw it was failing the test:

checking matlab 2016a:

I went back and checked mean, median, mode, etc., and they behave similarly with multidimensional empties. I see Matlab documentation about the simple empty matrix case, but nothing for ndim>2.

I'm assuming the old test was there because of former matlab behavior. Also . Matlab behavior gets strange depending on which dimension has 0 elements. For example:

I'm assuming we're venturing into the land of undocumented Matlab. Ignoring multidimensional empties I have a the last patch ready to upload now. If I could ignore that last case it's easy to catch ndims>2 and output a matching multidimensional empty. The last case is a little wonky, but it wouldn't take much more to catch that case. Do we want it to?

I'm assuming Matlab 'fixed' the std([])=NaN issue and the rest is unintentional. Other than function([])=NaN, is there anything documented I should be sure to follow? And then how much of the undocumented should I try to emulate?

I believe there was some recent list traffic on a similar topic of multidimensional empties and matlab compatibility, but my google fu is failing me.

I'll likely copy this over to maintainers-list later to get other opinions if needed.

Nick J.

Nicholas Jankowski <nrjank>
Mon 08 Aug 2016 05:45:14 PM UTC, comment #13:

was going to upload last patch for std([]) = NaN when I saw it was failing the test:

Nicholas Jankowski <nrjank>
Sun 07 Aug 2016 04:37:12 PM UTC, comment #12:

mode patch added. I noticed it already used a conditional return, so I did the same to catch a [] input. didn't know if conditional returns are considered good or bad form rather than a larger if/else like i did with median.

(file #38139)

Nicholas Jankowski <nrjank>
Sun 07 Aug 2016 04:07:04 PM UTC, comment #11:

var patch added

(file #38137)

Nicholas Jankowski <nrjank>
Sun 07 Aug 2016 04:01:10 PM UTC, comment #10:

updated patches attached for median, cov and mean preserving single/double

(file #38134, file #38135, file #38136)

Nicholas Jankowski <nrjank>
Sun 07 Aug 2016 03:46:51 PM UTC, comment #9:

ok, output for mode should follow:

also verified that the others all only have one output and do preserve single/double status as well. will post patches shortly.

Nicholas Jankowski <nrjank>
Sat 06 Aug 2016 06:08:28 PM UTC, comment #8:

Realized my other patches didn't preserve single/double type for the NAN. Will upload modified patches for this after confirming MATLAB behavior. Also, mode has multiple outputs. Need to test what those other outputs should be in MATLAB. If it's simple will have that up as well.

Nicholas Jankowski <nrjank>
Fri 05 Aug 2016 08:43:12 PM UTC, comment #7:

cov([]) produced [].

patch attached such that cov([]) == NaN

(file #38123)

Nicholas Jankowski <nrjank>
Fri 05 Aug 2016 08:36:09 PM UTC, comment #6:

mean([]) did produce a NaN, but only because it had a divide by zero hiding in there, producing some warnings. mean.m patch attached directly produces NaN without warnings.

(file #38122)

Nicholas Jankowski <nrjank>
Fri 05 Aug 2016 08:22:56 PM UTC, comment #5:

patch attached for median.m

(file #38121)

Nicholas Jankowski <nrjank>
Fri 05 Aug 2016 08:04:15 PM UTC, comment #4:

ok, my issue again. While I was testing the dev version of median I didn't notice the patch made changes to nth_element as well, and octave was testing with the release version of that.

Nicholas Jankowski <nrjank>
Fri 05 Aug 2016 07:56:51 PM UTC, comment #3:

the issue with median appears related to changes made in https://savannah.gnu.org/bugs/?47515

the original report indicates that in matlab

which is not the case in the version I'm testing with. In any case the dev version of median which appears to include the patch for that bug, errors out as mentioned below.

Nicholas Jankowski <nrjank>
Fri 05 Aug 2016 07:45:54 PM UTC, comment #2:

i was peeking at this and thought it would be a quick patch. Then I saw that median failed the boolean test. thought it may have been my edit, but reverted to unmodified dev and still:

also, checking Matlab output, it appears Matlab 2016a has some problems with a logical input to median:

it appears matlab median will accept a logical array as long as there are an odd number of values, but throw an error when it tries to do the divide by two for an even number.

octave appears to throw an error in either case, which should be fixed. I don't know if/how this applies to any other statistics functions.

Nicholas Jankowski <nrjank>
Wed 03 Aug 2016 05:53:32 PM UTC, comment #1:

Thank you for your bug report. Confirmed by Matlab's public docs on the cov, mean, median, mode, std, and var functions.

Mike Miller <mtmiller>
Project Administrator
Wed 03 Aug 2016 03:08:24 PM UTC, original submission:

Dear Gnu team,

Executing "median([])" in Octave produces an error. In MATLAB it merely returns an NaN.

Regards,
Shelby

Anonymous

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #41348:  mode.m added by nrjank (13KiB - text/plain - final(?) version of mode with code cleanup)
file #41346:  std.m added by nrjank (9KiB - text/plain - corrected std.m and var.m working for empty inputs and specified dim)
file #41347:  var.m added by nrjank (9KiB - text/plain - corrected std.m and var.m working for empty inputs and specified dim)
file #41344:  median.m added by nrjank (9KiB - text/plain - corrected median.m working for empty inputs and specified dim)
file #41343:  mean.m added by nrjank (9KiB - text/plain - corrected mean.m working for empty inputs and specified dim)
file #41342:  mode.m added by nrjank (13KiB - text/plain - corrected mode.m working for empty inputs and nargin>1)
file #39597:  cov_matlab_compatible.diff added by nrjank (21KiB - application/octet-stream - fully matlab compatible cov.m and changeset )
file #39598:  cov.m added by nrjank (17KiB - application/octet-stream - fully matlab compatible cov.m and changeset )
file #39502:  statistics_emptyinput_fix_allbutcov.diff added by nrjank (33KiB - application/octet-stream - single patch covering std, var, median, mode, and mean. )
file #38232:  modeNaN_v2.diff added by nrjank (7KiB - application/octet-stream)
file #38221:  meanNaN_v3.diff added by nrjank (6KiB - application/octet-stream)
file #38222:  varNaN_v2.diff added by nrjank (5KiB - application/octet-stream)
file #38223:  medianNaN_v3.diff added by nrjank (7KiB - application/octet-stream)
file #38224:  stdNaN_v1.diff added by nrjank (6KiB - application/octet-stream)
file #38139:  modeNaN.diff added by nrjank (2KiB - application/octet-stream)
file #38137:  varNaN.diff added by nrjank (1KiB - application/octet-stream - var.m patch)
file #38134:  medianNaN_v2.diff added by nrjank (3KiB - application/octet-stream - updated median , cov and mean patches to preserve single/double for output NaN)
file #38135:  meanNaN_v2.diff added by nrjank (1KiB - application/octet-stream - updated median , cov and mean patches to preserve single/double for output NaN)
file #38136:  covNaN_v2.diff added by nrjank (1023B - application/octet-stream - updated median , cov and mean patches to preserve single/double for output NaN)
file #38123:  covNaN.diff added by nrjank (868B - application/octet-stream)
file #38122:  meanNaN.diff added by nrjank (984B - application/octet-stream)
file #38121:  medianNaN.diff added by nrjank (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by urathai (Posted a comment)
  • -unavailable- added by nrjank
  • -unavailable- added by nrjank
  • -unavailable- added by nrjank
  • -unavailable- added by siko1056 (Updated the item)
  • -unavailable- added by nrjank (Posted a comment)
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by None (Submitted the item)
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 25 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 26 Jul 2017 08:01:34 PM UTCnrjankAttached File-=>Added mode.m, #41348
    Wed 26 Jul 2017 07:52:16 PM UTCnrjankAttached File-=>Added std.m, #41346
      Attached File-=>Added var.m, #41347
    Wed 26 Jul 2017 07:19:04 PM UTCnrjankAttached File-=>Added median.m, #41344
    Wed 26 Jul 2017 07:02:00 PM UTCnrjankAttached File-=>Added mean.m, #41343
    Wed 26 Jul 2017 06:16:12 PM UTCnrjankAttached File-=>Added mode.m, #41342
    Sun 29 Jan 2017 03:52:25 AM UTCnrjankAttached File-=>Added cov_matlab_compatible.diff, #39597
      Attached File-=>Added cov.m, #39598
    Wed 18 Jan 2017 07:02:35 PM UTCnrjankAttached File-=>Added statistics_emptyinput_fix_allbutcov.diff, #39502
    Wed 18 Jan 2017 04:10:23 PM UTCnrjankCarbon-Copy-=>Added jordigh
      Carbon-Copy-=>Added jwe
      Carbon-Copy-=>Added
    Tue 25 Oct 2016 02:13:02 PM UTCsiko1056StatusConfirmed=>Patch Submitted
    Tue 16 Aug 2016 03:42:07 PM UTCnrjankAttached File-=>Added modeNaN_v2.diff, #38232
    Mon 15 Aug 2016 08:21:06 PM UTCnrjankAttached File-=>Added meanNaN_v3.diff, #38221
      Attached File-=>Added varNaN_v2.diff, #38222
      Attached File-=>Added medianNaN_v3.diff, #38223
      Attached File-=>Added stdNaN_v1.diff, #38224
    Sun 07 Aug 2016 04:37:12 PM UTCnrjankAttached File-=>Added modeNaN.diff, #38139
    Sun 07 Aug 2016 04:07:04 PM UTCnrjankAttached File-=>Added varNaN.diff, #38137
    Sun 07 Aug 2016 04:01:10 PM UTCnrjankAttached File-=>Added medianNaN_v2.diff, #38134
      Attached File-=>Added meanNaN_v2.diff, #38135
      Attached File-=>Added covNaN_v2.diff, #38136
    Fri 05 Aug 2016 08:43:12 PM UTCnrjankAttached File-=>Added covNaN.diff, #38123
    Fri 05 Aug 2016 08:36:09 PM UTCnrjankAttached File-=>Added meanNaN.diff, #38122

    Back to the top


    Powered by Savane 3.1-cleanup1