search

brycebaril / node-stats-lite

A light statistical package that operates on Arrays.
MIT License
74 stars 11 forks source link

Mode returning NaN for array of numbers w/ multiple Modes #1

Closed netpoetica closed 9 years ago

netpoetica commented 10 years ago 
var arr = [1022546947, 92816789, 2723915, 412069328, 439592640, 3380925, 74048396, 2659505, 7744922, 332453996, 185203296, 1642275096, 78737236, 3774332, 2546685453, 93804009, 68343469, 6232930, 83200134, 1007722, 4825030660];

console.log(stats.mode(arr));
// -> NaN

Looking into it now but I figured I should report asap, will let you know if I figure it out

Node v0.10.21, Mac OSX

netpoetica commented 10 years ago

The culprit is https://github.com/brycebaril/node-stats-lite/blob/master/stats.js#L74

Shouldn't this function just return an array of modes if there are multiple modes?

brycebaril commented 9 years ago

I have no idea how I didn't see this until today :blush:

When I wrote this I sided toward the solution that if the dataset has multiple modes it has no mode.

This provided a few things for me:

  1. It was easier :smile_cat:
  2. It provided a more consistent return type (1 number) vs sometimes a number, sometimes an Array
  3. It avoided the pathological case where mode(dataset) =~ dataset
  4. It was what I needed at the time.

However, I'm pretty ok with switching the behavior of this now. I have some small concerns about allowing multiple return types and V8 optimizations, but I don't know how often mode is being used in people's hot loops.

As for changing the return type, most likely people were having to look for NaN as is, so looking for an Array is not really a significant code change.

brycebaril commented 9 years ago

Pushed as v2.0.0

mode now returns an ES6 Set of the modes for multi-modal datasets