Combine and optimize mad! and mad

[LilithHafner]

Generators don't have eltype (have eltype Any) so this explicitly makes an Any[] which, ironically, isn't even reused in the following code due to the isconcretetype check. A standard collect is better.

julia> @btime mad((i for i in 1:100)) # before
  13.956 μs (321 allocations: 9.59 KiB)
37.065055462640046

julia> @btime mad((i for i in 1:100)) # after
  1.621 μs (5 allocations: 4.38 KiB)
37.065055462640046

[LilithHafner]

Bump, this seems pretty clearcut to me.

[andreasnoack]

Maybe a good idea to add a test that counts the allocations of this case.

[mschauer]

Trying to understand the code. Is median! guaranteed to preserve the elements, just change their order?

[LilithHafner]

Is median! guaranteed to preserve the elements, just change their order?

No. median! "may overwrite the input vector." as per the docstring. However, its implementation does satisfy this property and as this code is in StatsBase.jl, it is okay to depend on that implementation detail. Perhaps we should strengthen the claim in median!'s docstring if folks outside of StatsBase are depending on that behavior.

[LilithHafner]

Maybe a good idea to add a test that counts the allocations of this case.

Can add a rough test, but this calls median! which calls partialsort! which does not have a guaranteed limit on number of allocations.

[mschauer]

And then, next question, if the center is float and the array is integer, how can x2 = collect(x) can hold the differences between the elements and the center? E.g. mad(Integer[1,1,2,2])?

[LilithHafner]

Good catch. For collections that are not AbstractArrays but do have concrete eltypes that cannot hold their own middle this will error. I'll have to make a larger diff to cover all cases without any regressions.

[andreasnoack]

E.g. mad(Integer[1,1,2,2])?

I don't think there is an issue with that. There is another branch https://github.com/JuliaStats/StatsBase.jl/pull/839/files#diff-f1f51914b0e1897b4ede4d6024d011c6aa91c7e74e483bae63a33f1e892d8eb9R545 that doesn't work in-place if the element type isn't concrete.

[LilithHafner]

The counterexample is mad(Iterators.repeated(4, 10)).

[mschauer]

There is also no point in collecting anything into x2 if c is provided. Maybe it's better to rewrite than to repair.

[LilithHafner]

I also arrived at that conclusion and have done a rewrite. Indeed, I have avoided collecting anything into x2 in all cases.

[LilithHafner]

I'd like to rebase this after #841 so that the diff can be more clear here.

[LilithHafner]

Less source code, more tests, more concrete types, fewer allocations, faster runtime, and hopefully not terribly buggy :) What more can you ask for?

This is ready for review!

[mschauer]

Let me just understand this: If U is an Int and T is Float64, this would allow to overwrite the Int array by reinterpreting it filling it with intermediate floats, but x interpreted as Ints is garbage afterwords. We should warn that x cannot be used afterwards in mad!

[LilithHafner]

The docstring begins with "Compute the median absolute deviation (MAD) of array x around center (by default, around the median), overwriting x in the process." so this warning does exist.

x can be reused, it has simply been overwritten with gibberish.

Previously, mad! began with x .= abs.(x .- center) and then partially sorted x, which makes the contents of x fairly unusable, though not quite as bizarre as x can end up now.

We can make the warning more visible if you'd like, but it's also contained in the function name itself. mad! clearly mutates its input, the warning is just that it probably mutates it in a way you aren't expecting.

[mschauer]

Ah, I am happy with "overwriting x in the process"

[LilithHafner]

Is there anything else needed from me here?

[mschauer]

No, if there is no protest I will merge this.