JuliaPlots / StatsPlots.jl

Statistical plotting recipes for Plots.jl
Other
437 stars 88 forks source link

Where to put custom DSP plots? #147

Open standarddeviant opened 6 years ago

standarddeviant commented 6 years ago

Hello,

I apologize if this isn't the right place for this discussion. I would like to get some plot functions/recipes in to a package that is aware of custom types in DSP.jl, there's a discussion here: https://github.com/JuliaDSP/DSP.jl/issues/203

I promised to follow up with StatPlots.jl devs on a question from that discussion: Do people think StatPlots.jl would be a good home for DSP.jl-aware plots?

UPDATE: for reference, this is the particular plotting function - it's similar to the freqz visualization in Matlab:

import Plots
function Plots.plot(fc::FilterCoefficients, w=linspace(0, pi, 500); fs=2, 
                    domag=true, dophase=true, 
                    doimp=false, impn=500, 
                    dostep=false, stepn=500, size=(800, 600))
    # calculate plottable vectors
    if domag || dophase
        h = freqz(fc, w)
    end
    if doimp
        ir = impz(fc, impn)
    end
    if dostep
        sr = stepz(fc, stepn)
    end

    # make vector of plots
    theplots = Plots.Plot[]
    hz = w ./ pi .* (fs/2)
    if domag
        mag = 20*log10.(abs.(h))
        ylims = [maximum(mag)-70, maximum(mag)+10] 
        push!(theplots, plot(hz, mag, lw=2, c=:blue, ylims=ylims, ylabel="dB (mag)"))
    end
    if dophase
        phase = unwrap(angle.(h))
        push!(theplots, plot(hz, phase, lw=2, c=:red, ylabel="rad (phase)"))
    end
    if doimp
        push!(theplots, plot((0:impn-1)./fs, ir, lw=2, c=:green, ylabel="imp resp"))
    end
    if dostep
        push!(theplots, plot((0:stepn-1)./fs, sr, lw=2, c=:purple, ylabel="step resp"))
    end

    # add a title
    plot!(theplots[1], title="filter response")
    # add xlabel to the last plot for space saving
    plot!(theplots[end], xlabel="Hz (freq-plots) or Seconds (time-plots)")

    plot(theplots..., layout=(sum([domag, dophase, doimp, dostep]), 1), legend=false, size=size)
end

It can be demoed with:

using DSP
fs = 10
bpf = digitalfilter(Bandpass(1, 2; fs=fs), Butterworth(4))
plot(bpf, doimp=true, dostep=true, size=(900,500))

I'm reading up on recipes to try to implement the above as using macros from RecipesBase.jl.

piever commented 6 years ago

DSP seems lightweight enough that adding a dependency here shouldn't be a big deal. I'm in favor to adding here a dependency to have a recipe to plot objects from signal processing. @mkborregaard , what do you think?

mkborregaard commented 6 years ago

Here or in PlotRecipes, whereever you think is best.

mkborregaard commented 6 years ago

I agree with the discussion on the issue that a more relevant place to put it would be in DSP itself, but failing that it'd be fine to have it here or in PlotRecipes. I don't think it will interact with any functionality here in StatPlots which might be an argument for PlotRecipes, but on the other hand that's a dependency-heavy package.

ssfrr commented 6 years ago

I think StatsPlots would be a weird place for this functionality - it seems unrelated to all things statistical. I guess PlotRecipes would be reasonable, but then you'd need to add a dependency to DSP to it, which doesn't seem like a scalable way to do things (if people keep doing that, PlotRecipes ends up being this sprawling octopus package with dependencies on all sorts of unrelated package).

mkborregaard commented 6 years ago

I agree with that completely. Adding this recipe to DSP itself is by far the most sustainable approach, and in fact it comes with no disadvantages to anyone. However, some developers on the statistical packages disagree with adding plot recipes on principle; discussions like the one on that issue has been had more times than I can count on these packages. StatPlots and PlotRecipes represent a compromise for still making recipe functionality available and discoverable to the community, with exactly the consequence you describe, at least for PlotRecipes. Now we've decided that StatPlots should depend mainly on the same packages as Stats.jl that will be more coherent.

Any good suggestions for resolving this are greatly appreciated.

mkborregaard commented 6 years ago

Did you put this in DSP @standarddeviant ? I see the linked issue is closed