Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyword arguments in convert_arguments? #837

Open
ChrisRackauckas opened this issue Feb 6, 2021 · 22 comments
Open

Keyword arguments in convert_arguments? #837

ChrisRackauckas opened this issue Feb 6, 2021 · 22 comments

Comments

@ChrisRackauckas
Copy link
Contributor

For the DiffEq type recipes, we need to have custom keyword arguments from users and to set a few other plotting attributes (labels). Is that able to be done?

@pbouffard
Copy link
Contributor

Maybe #893 is related?

@SimonDanisch
Copy link
Member

Can you specify what keyword arguments are most important to set for those recipes?

@ChrisRackauckas
Copy link
Contributor Author

https://github.com/SciML/SciMLBase.jl/blob/master/src/solutions/solution_interface.jl#L151-L164

denseplot, plotdensity, tspan, and vars are all commonly used.

@Datseris
Copy link
Contributor

Datseris commented Apr 1, 2021

I would also need custom keywords for JuliaDynamics packages, so I'm subscribing to this issue!

@VarLad
Copy link
Contributor

VarLad commented May 7, 2021

Would love to see the Makie support for DifferentialEquations

@SimonDanisch
Copy link
Member

denseplot, plotdensity, tspan, and vars are all commonly used.

I have no idea what those should do as keyword arguments :D Are those referring to plot types?

So convert_arguments actually does support handling kw_args, even though its a bit complicated and I'm not sure if we want to keep it that way...
The bigger question is, should convert_argument or any other recipe be able to mess with the axis?
Modifying the axis has large implications which aren't easy to handle elegantly. E.g., if some plot recipe changes the ticklabels, how can you plot other types into the same axis?
On the other hand, if you want to create density plots etc, convert_arguments isn't really the tool anyways, but a type recipe should be used...
If we can find better examples of what's actually needed, I can try to figure out how we can translate it to makie recipes

@ChrisRackauckas
Copy link
Contributor Author

I have no idea what those should do as keyword arguments

Of course you (Makie) don't, because Makie doesn't supply their interpretation, the type-recipe does. I want my type recipe to add new keyword arguments to the pipeline, since the Makie keyword arguments are insufficient to describe things like "how many points to interpolate the continuous function by", nor should it try to do all of them. At some point, types need the ability to define new arguments.

Modifying the axis has large implications which aren't easy to handle elegantly. E.g., if some plot recipe changes the ticklabels, how can you plot other types into the same axis?

This is still a major misunderstanding, since you're still thinking about keyword arguments Makie has, not the ones I need to add.

On the other hand, if you want to create density plots etc, convert_arguments isn't really the tool anyways, but a type recipe should be used...

What's the proper way to do this?

If we can find better examples of what's actually needed, I can try to figure out how we can translate it to makie recipes

The DiffEq example is a good one that is in wide use and has a few necessary kwargs.

@SimonDanisch
Copy link
Member

So as I said, getting kw arguments into a recipe isn't the problem, doing something with them is..

This is still a major misunderstanding

So I see mainly things like xlims, ylims, xguide, yguide etc being set in that recipe... Aren't those for the axis?

@ChrisRackauckas
Copy link
Contributor Author

ChrisRackauckas commented May 21, 2021

So I see mainly things like xlims, ylims, xguide, yguide etc being set in that recipe... Aren't those for the axis?

Yes, but that's a red harring and completely unrelated to the issue. The issue is what to do with the plotdensity or vars kind of keyword arguments. I could do without the automated plot labeling if Makie recipes don't support it, but the DiffEq recipe cannot exist without vars.

@SimonDanisch
Copy link
Member

It's not very well documented and may change a bit in 1.0, but this has been working basically forever:

struct AbstractTimeseriesSolution
    results::Vector{Float32}
end

# Uh, this looks like a new bug that needs fixing
Makie.plotsym(::Type{Any}) = :plot

function Makie.plot!(plot::Makie.Plot(AbstractTimeseriesSolution))
    arg1_observable = plot[1] # first argument from plot(args...) as an observable
    # plot contains any keyword arguments that you pass to plot(series; kw...)
    result = if haskey(plot, :var)
        plot.var[] .* arg1_observable[].results
    else
        arg1_observable[].results
    end
    density!(plot, result)
end

plot(AbstractTimeseriesSolution(rand(Float32, 10)), var=10)

image
I'm just using [] everywhere to dereference the observable, to make everything animatable, one would need to use this:

result = lift(plot.var, plot[1]) do var, arg1
   arg1 .* var
end

Or

@lift $(plot.var) .* $(plot[1])

It's not as nice when giving the kw args defaults etc... But those are things we want to improve in the future

@ChrisRackauckas
Copy link
Contributor Author

That's all the ammo we need, thanks! @YingboMa is that enough to finish your prototype example from yesterday?

using Makie
struct Solution{T,U}
    ts::T
    us::U
end
@recipe(SolutionPlot) do scene
    Theme(
          ccc = :red,
          shift = 0
         )
end
function Makie.plot!(sp::SolutionPlot{<:Tuple{<:Solution}})
    sol = sp[1]
    ts, us = sol[].ts, sol[].us
    lines!(sp, ts, us .+ sp.shift[], color=sp.ccc[])
    sp
end
sol = Solution(1:10, rand(10))
solutionplot(sol; shift=100, ccc=:darkblue)

Also, does a recipe like this automatically compose with other recipes?

@Datseris
Copy link
Contributor

Datseris commented May 21, 2021

result = if haskey(plot, :var)

But this kind of approach seems far from optimal though, and unlikely to stay as a stable interface. Why is it that

function Makie.plot!(plot::Makie.Plot(AbstractTimeseriesSolution); vars = (1, 2, 3))

is initself not possible?

@ffreyer
Copy link
Collaborator

ffreyer commented May 21, 2021

You can do

function plot!(plot::SomePlot{MyType})
    haskey(plot, :var) || (plot.attributes[:var] = Node(default)
    ...
end

to add it for a specific plot data type without adjusting the general default Attributes I suppose.

@SimonDanisch
Copy link
Member

Plot implements the dict interface, so this would be preferrable:

function Makie.plot!(plot::Makie.Plot(AbstractTimeseriesSolution))
    # plot contains any keyword arguments that you pass to plot(series; kw...)
    var = get(plot, :var, Node(5))
    density!(plot, @lift $var .* $(plot[1]).results)
end

@Datseris
Copy link
Contributor

Datseris commented May 21, 2021

# plot contains any keyword arguments that you pass to plot(series; kw...)

Oookay, this _partly) solves the issue. The default keywords still remains unfortunately. And sometimes the defaults depend on the input data.

@SimonDanisch
Copy link
Member

Ok, there was another bug for composing type recipes :D
#965

With those fixes this works:

using GLMakie

struct AbstractTimeseriesSolution
    results::Vector{Float32}
end

function Makie.plot!(plot::Makie.Plot(AbstractTimeseriesSolution))
    # plot contains any keyword arguments that you pass to plot(series; kw...)
    var = get(plot, :var, Node(5))
    density!(plot, @lift $var .* $(plot[1]).results)
end

struct Test2
    series
end 

function Makie.plot!(plot::Makie.Plot(Test2))
    arg1 = plot[1]
    scatter!(plot, arg1[].series)
    ser = AbstractTimeseriesSolution(arg1[].series)
    plot!(plot,ser, var=10)
end

plot(Test2(rand(Float32, 10)))

image

@YingboMa
Copy link

BTW, in the above Solution example, how can we make scatter(Solution(1:10, rand(10))) as well as plot(Solution(1:10, rand(10))) work in a way that preserves the kwargs?

@devmotion
Copy link
Contributor

I think it would still be nice to be able to access keyword arguments in convert_arguments. Consider e.g. the conversions in ReliabilityDiagrams (https://github.com/devmotion/ReliabilityDiagrams.jl/blob/main/src/conversions.jl):

function AbstractPlotting.convert_arguments(
    ::Type{<:Reliability},
    probabilities::AbstractVector{<:Real},
    frequencies::AbstractVector{<:Real},
)
    return (map(Point2f0, probabilities, frequencies),)
end

function AbstractPlotting.convert_arguments(
    ::Type{P},
    probabilities::AbstractVector{<:Real},
    outcomes::AbstractVector{Bool},
    algorithm,
) where {P<:Reliability}
    meanprobabilities, meanfrequencies, _ = reliability_summary(
        probabilities, outcomes, algorithm
    )
    return AbstractPlotting.convert_arguments(P, meanprobabilities, meanfrequencies)
end

Users should be able to provide pre-binned probabilities and frequencies, so the vector of Point2f0s is the data endpoint that should be passed to the plotting function plot!. However, for convenience, it would be nice if users should be able to also provide probabilities and outcomes that are not binned and binned in convert_arguments, depending on the chosen algorithm, resulting in the same data format for plot!. Since keyword arguments are not available at this stage I currently require it to be a positional argument.

Now I am in the process of adding another functionality (so-called consistency bars) which are computed by a resampling procedure that the user should be able to adjust. To limit the number of arguments my approach so far has been to use a custom Options struct or possible a NamedTuple (not sure yet) instead of algorithm in the example. But this seems wrong and just like reinventing keyword arguments. So I wonder if I just use Makie incorrectly. Is the idea that instead of convert_arguments one should just implement all this processing pipeline in plot!?

In my opinion, this seems a bit weird since I would expect convert_arguments to allow me to convert all arguments to the format that plot! can deal with. While I assume (haven't implemented it) one can work around this issue by shifting all computations to plot!, I think the plot! approach is more limiting since it is not as easily extendable: if I want to support other input types, I have to modify plot! instead of just adding a convert_arguments function with the correct output data it seems.

@SimonDanisch
Copy link
Member

So, you can actually access kw-args in convert_arguments, but it hasn't really been advertised that much, because I'm not sure if I like how all of this works :D
But this does work:

struct Solution
    data 
end

Makie.used_attributes(::Any, x::Solution) = (:attribute,)

# Convert for all point based types (lines, scatter)
function Makie.convert_arguments(p::Makie.PointBased, x::Solution; attribute = 1.0)
    return convert_arguments(p, x.data  .* attribute,)
end

scatter(Solution(rand(4)), attribute=10)

We're a bit greedy with not passing all attributes to convert_arguments since any additional observable passed to this will run convert_arguments whenever it changes...
So if we just pass all of the attributes to it by default, you would run convert_arguments everytime one changes the color, even if it doesn't depend on the color...
This has led to the somewhat awkward Makie.used_attributes(::Any, x::Solution) = (:attribute,) ... We could have this as a macro or something like that to make it more user friendly...

@devmotion
Copy link
Contributor

Ah nice, I didn't know used_attributes exists! This seems like a useful functionality, maybe it should be made part of the official API and documented (if it isn't yet)? Maybe also something like convert_attributes or convert_arguments_attributes would be more descriptive?

BTW is the first argument the plot type?

@SimonDanisch
Copy link
Member

BTW is the first argument the plot type?

Yes ;)

@devmotion
Copy link
Contributor

Thanks 👍 And I guess Makie specializes on the arguments to avoid unnecessary recomputations? E.g., if in my example above I define

function Makie.used_attributes(::Reliability, ::AbstractVector{<:Real}, ::AbstractVector{<:Bool})
    return (:binning, :bars)
end

I would be able to use these keyword arguments in convert_arguments for these specific types but for other input arguments changing these keyword arguments would not retrigger the convert_arguments pipeline?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants