search

folz / math

The missing Math module for Elixir.
https://hex.pm/packages/math
Other
104 stars 12 forks source link

Histogram #28

Closed pcarvsilva closed 2 years ago

pcarvsilva commented 2 years ago

Adding Histogram functions, probably helpful for livebook stuff

Qqwy commented 2 years ago

I clicked the wrong button when I wanted to reply with a comment; apologies.

Also, sorry for the very long wait before my response.

@pcarvsilva do you still want to pursue this? Histogram functionality is within the scope of the library. However, I have two questions:

  1. the tests you have written are currently failing with a compile error:
** (CompileError) test/math/enum_test.exs:23: undefined function min/1 (expected Math.EnumTest to define such a function or for it to be imported, but none are available)

It is not entirely clear to me how this error should be fixed.

  1. I have the following remarks w.r.t. the invariants of the histogram function:
    • Math.Enum.histogram(enumerable, 0) results in an ArithmeticError. It would be better to raise an ArgumentError that hints at the caller mis-using the function.
    • Passing a float to the number of buckets does not crash but it probably should as the result is meaningless.
    • Calls like Math.Enum.histogram([1,2,3,1,2,3], 100) do not result in 100 buckets (with most of them having 0 as value), but only with three buckets.
    • Similarily, Math.Enum.histogram([1,2,3,1,2,3], 1) results in 2 buckets, and Math.Enum.histogram([1,2,3,1,2,3], 2) results in 3 buckets.
    • Returning a map is better than a keyword list, because all bucket keys are separate.