Add S.mean that calculates mean of values of a foldable

[jdudek]

I’ve implemented the mean function, as it’s been suggested in #194.

The tests are roughly based on product tests. One thing I couldn’t figure out is how to put a type constraint on the argument of type f, i.e. how to specify the function accepts only foldables of finite numbers. I’ve noticed neither product nor sum enforce this restriction on their argument, only on their results (even though the comments suggest they do: product :: Foldable f => f FiniteNumber -> FiniteNumber).

Passing a foldable with an infinite number to mean does not typecheck, but it fails on the call to sum, which makes the message a bit misleading.

[davidchambers]

Thank you for the pull request, @jdudek! It's exciting to receive a pull request from a new contributor. :)

One thing I couldn’t figure out is how to put a type constraint on the argument of type f, i.e. how to specify the function accepts only foldables of finite numbers.

It's not currently possible, but we'll be able to add this constraint in #232 thanks to introduction of $.UnaryTypeVariable in sanctuary-js/sanctuary-def#79.

We can simplify the implementation quite a bit. If we define mean in terms of reduce_ we can avoid type checks. Something along these lines should work:

var result = reduce_(
  function(acc, n) { return {total: acc.total + n, count: acc.count + 1}; },
  {total: 0, count: 0},
  foldable
);
return result.count === 0 ? Nothing() : Just(result.total / result.count);

[jdudek]

Thanks for the reply. I thought about accumulating both sum and count in one fold, but it seemed like it would introduce unnecessary object allocations. So today I ran a simple benchmark and it turns out your solution is not only more concise, but also significantly faster (both with typechecking on and off)—assuming my microbenchmark makes any sense :) I also tested with much larger arrays and the results where similar.

I’ve updated the implementation and added a test for the case with infinite arguments.

[kedashoe]

but it seemed like it would introduce unnecessary object allocations

I think this is still fair to say. Any objections from anyone to something like

var result = reduce_(
  function(acc, n) {
    acc.total += n;
    acc.count += 1;
    return acc;
  },
  {total: 0, count: 0},
  foldable
);

?

[davidchambers]

Thanks for the updates, @jdudek. They look good. I think @kedashoe's version with local mutation is even better. Let's go with that. :)

[davidchambers]

👾

[jdudek]

[davidchambers]

💥

[jdudek]

I couldn’t resist running the benchmark with @kedashoe’s version and it actually is faster:

Results
-> benchmark.js
    mean (jd) x 2,728 ops/sec ±1.50% (86 runs sampled)
    mean (dc) x 6,699 ops/sec ±1.27% (86 runs sampled)
    mean (kedashoe) x 8,739 ops/sec ±1.42% (82 runs sampled)

[davidchambers]

Thanks! I'd like to request one final change. Could you move the definition below that of S.div? It doesn't seem right to me to include S.mean between S.mult and S.div (whereas it does make sense for S.product to be grouped with S.mult). Also, please squash the commits. :)

[jdudek]

✅

[davidchambers]

🌳 Very nice pull request. Thanks again!

[davidchambers]

$ npm install [email protected]

$ node
> const S = require('sanctuary')
undefined
> S.mean
mean :: Foldable f => f FiniteNumber -> Maybe FiniteNumber