Add ECDF plot

[sethaxen]

This PR adds

• a plotting function ecdfplot for plotting empirical cumulative distribution functions
• conversions for StatsBase.ECDF objects

I'm not sure how/if to document the conversion functions.

Example usage:

using CairoMakie, StatsBase

fig = Figure(resolution = (1000, 1000))
ecdfplot(fig[1, 1], randn(200))

x = randn(200)
ecdfplot(fig[1, 2], x, color = (:blue, 0.3))
ecdfplot!(fig[1, 2], x, color = :red, npoints=10)

x = rand(200)
w = @. x^2 * (1 - x)^2 
ecdfplot(fig[2, 1], x)
ecdfplot!(fig[2, 1], x; weights = w, color=:orange)

plot(fig[2, 2], ecdf(randn(200)))
plot!(fig[2, 2], 0..Inf, ecdf(randn(200)); color=:blue)
scatter!(fig[2, 2], -3:0.5:0, ecdf(randn(20)); color=:magenta)

fig

[jkrumbiegel]

Looks very good to me, thanks for the contribution! Any thoughts @piever or @SimonDanisch?

[piever]

Looks good to me to, thanks for the nice contribution! I've left a very minor comment.

[mschauer]

Not a very constructive comment, but I am quite convinced that the right way to set this up would be a plot recipe for something a Stats package returns and not a function that receives the raw data, then calls the stats package function on it and then finally plots it.

[sethaxen]

Not a very constructive comment, but I am quite convinced that the right way to set this up would be a plot recipe for something a Stats package returns and not a function that receives the raw data, then calls the stats package function on it and then finally plots it.

The current convention for distributions/samples is to have both. For example, there's plot(::StatsBase.Histogram) and histogram, plot(::KernelDensity.UnivariateKDE) and density, and plot(::Distributions.QQPair) and qqplot. This is handy because the user is probably more interested in a statistical plot than in the object itself, and then they don't need to worry about installing the specific package and learning its interface for constructing the objects, as well as learning how Makie's converter works.

But there might be a way to delete some code here to make the plotting of ECDF objects and ecdfplot more uniform. Looks like qqplot has a simpler implementation.

[piever]

But there might be a way to delete some code here to make the plotting of ECDF objects and ecdfplot more uniform. Looks like qqplot has a simpler implementation.

I also think so, I have added some suggestions on how to unify the implementation (it basically does everything via convert_arguments).

[sethaxen]

@piever for some reason with the recommended changes I get the error:

ERROR: PlotMethodError: no ecdfplot method for arguments (::Vector{Point{2, Float32}}). To support these arguments, define
  plot!(::Combined{Makie.ecdfplot, S} where S<:Tuple{Vector{Point{2, Float32}}})

[piever]

Mhm, I confess I'm a bit rusty on the details on how this works. Does the method below get called?

function convert_arguments(::Type{<:ECDFPlot}, x::AbstractVector; npoints=10_000, weights=StatsBase.Weights(Float64[]))
    ecdf = StatsBase.ecdf(x; weights=weights)
    return convert_arguments(Stairs, ecdf, npoints=npoints)
end

In that case, I thought that would call the convert_arguments(P::PlotFunc, ecdf::StatsBase.ECDF; npoints=10_000) which returns a PlotSpec, so Makie should figure out that the plot type ECDFPlot has been replaced by Stairs. Do you get a PlotSpec (where the plot type is Stairs) if you run convert_arguments(ECDFPlot, x)?

EDIT: I've tested locally and indeed convert_arguments(ECDFPlot, x) returns PlotSpec{Stairs}(args...; kwargs...). @SimonDanisch when that is the case (eg convert_arguments(ECDFPlot, x) returns a PlotSpec{Stairs} object) shouldn't the Stairs recipe be called? Here it looks like it's still trying to call the ECDFPlot recipe.

[SimonDanisch]

Can you give me a quick MWE, so I can try it out and see what's going on?

[piever]

So, MWE would be as follows:

julia> using Makie

julia> @recipe(MyPlot) do scene
           Theme(;
               default_theme(scene, Scatter)...
           )
       end

julia> Makie.convert_arguments(::Type{<:MyPlot}, x) = PlotSpec{Scatter}(convert_arguments(Scatter, x)...)

julia> myplot(1:3)
ERROR: PlotMethodError: no myplot method for arguments (::Vector{Point{2, Float32}}). To support these arguments, define
  plot!(::Combined{myplot, S} where S<:Tuple{Vector{Point{2, Float32}}})
Available methods are:

Stacktrace:
 [1] _plot!(p::Combined{myplot, Tuple{Vector{Point{2, Float32}}}})
   @ Makie C:\Users\pietro\.julia\packages\Makie\PFSZS\src\interfaces.jl:347
 [2] plot!(p::Combined{myplot, Tuple{Vector{Point{2, Float32}}}})
   @ Makie C:\Users\pietro\.julia\packages\Makie\PFSZS\src\interfaces.jl:342
 [3] plot!(scene::Scene, P::Type{Combined{myplot, Tuple{UnitRange{Int64}}}}, attributes::Attributes, input::Tuple{Observable{UnitRange{Int64}}}, args::Observable{Tuple{Vector{Point{2, Float32}}}})
   @ Makie C:\Users\pietro\.julia\packages\Makie\PFSZS\src\interfaces.jl:428
 [4] plot!(scene::Scene, P::Type{Combined{myplot, ArgType} where ArgType}, attributes::Attributes, args::UnitRange{Int64}; kw_attributes::Base.Iterators.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:show_axis,), Tuple{Bool}}})
   @ Makie C:\Users\pietro\.julia\packages\Makie\PFSZS\src\interfaces.jl:339
 [5] plot(P::Type{Combined{myplot, ArgType} where ArgType}, args::UnitRange{Int64}; axis::NamedTuple{(), Tuple{}}, figure::NamedTuple{(), Tuple{}}, kw_attributes::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ Makie C:\Users\pietro\.julia\packages\Makie\PFSZS\src\figureplotting.jl:28
 [6] plot
   @ C:\Users\pietro\.julia\packages\Makie\PFSZS\src\figureplotting.jl:18 [inlined]
 [7] myplot(args::UnitRange{Int64}; attributes::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ Main C:\Users\pietro\.julia\packages\MakieCore\S8PkO\src\recipes.jl:31
 [8] myplot(args::UnitRange{Int64})
   @ Main C:\Users\pietro\.julia\packages\MakieCore\S8PkO\src\recipes.jl:31
 [9] top-level scope
   @ REPL[10]:1

IMO, once convert_arguments(MyPlot, x) returns PlotSpec{Scatter}(args...), Makie should try and plot a Scatter. This would be convenient in this case, because ECDFPlot should really just do some argument preprocessing and call Stairs, but that can all be done in convert_arguments. Otherwise, one has to write a bit of a silly plotting method for ECDFPlot with a lift call inside to do the argument conversion.

[SimonDanisch]

Hm weird, It seems the S from PlotSpec{S} is only respected from plottype if plottype feels like it...
Intuitively, the S should always take prevalence:
https://github.com/JuliaPlots/Makie.jl/blob/master/src/interfaces.jl#L153

[SimonDanisch]

changing that to not applying plottype makes it work ... Not sure for what case this was introduced 🤔

[SimonDanisch]

Do you want to make that change in this PR, and see if it passes tests?

function apply_convert!(P, attributes::Attributes, x::PlotSpec{S}) where S
    args, kwargs = x.args, x.kwargs
    # Note that kw_args in the plot spec that are not part of the target plot type
    # will end in the "global plot" kw_args (rest)
    for (k, v) in pairs(kwargs)
        attributes[k] = v
    end
    return (S, args) # this change, line 153 in interfaces.jl
end

[sethaxen]

Everything seems to work, save for one method, and it's not clear to me why:

julia> using GLMakie, StatsBase

julia> x = randn(200);

julia> w = weights(rand(200));

julia> plot(ecdf(x; weights=w)) # this is fine

julia> ecdfplot(x) # also fine

julia> ecdfplot(x; weights=w)
Error showing value of type Makie.FigureAxisPlot:
ERROR: MethodError: no method matching gl_convert(::Weights{Float64, Float64, Vector{Float64}})
Closest candidates are:
  gl_convert(::GLMakie.GLAbstraction.AbstractLazyShader, ::Any) at /Users/sethaxen/projects/Makie.jl/GLMakie/src/GLAbstraction/GLShader.jl:203
  gl_convert(::Function, ::Any) at /Users/sethaxen/projects/Makie.jl/GLMakie/src/GLAbstraction/GLUniforms.jl:266
  gl_convert(::StaticArrays.SMatrix{N, M, T, L} where L) where {N, M, T} at /Users/sethaxen/projects/Makie.jl/GLMakie/src/GLAbstraction/GLUniforms.jl:230
  ...

[piever]

Everything seems to work, save for one method, and it's not clear to me why

It seems like an another bug that the keyword argument gets passed to the backend even if it is in used_attributes. Does it make a difference if it is parte of the theme of the recipe ECDFPlot or not?

[sethaxen]

Does it make a difference if it is parte of the theme of the recipe ECDFPlot or not?

No, making it part of the theme doesn't change anything, with or without the used_attributes definition.

[piever]

Turns out those used_attributes still got passed to the backend, should be fixed by #1456

[sethaxen]

That fixed it for me! This should be ready for final review then.

[piever]

LGTM!