MakieOrg / Makie.jl

Interactive data visualizations and plotting in Julia
https://docs.makie.org/stable
MIT License
2.38k stars 302 forks source link

Tracking plot types that don't yet have all `lift`s linked to the plot object #4304

Closed asinghvi17 closed 4 days ago

asinghvi17 commented 1 week ago

A while ago we introduced a mechanism to track Observable pipelines (created by lift, map, on, and onany) and associate them with a plot so that they get garbage collected along with that plot. This is done by providing the plot as the second argument to the function, for example lift(f, plot::Plot, args...).

Most Makie recipes have already transitioned, however some still have to add this tracking. It's as simple as adding the plot (or whatever name the plot! function has it under) as the second argument to the lift or similar functions within plot!.

Specifically,

- lift(x, y, z) do x, y, x
+ lift(plot, x, y, z) do x, y, z
...
end

or

- lift(some_function, x, y, z)
+ lift(some_function, plot, x, y, z)

I made a PR to do this for contour in #4303, which could be a nice reference.

All the recipes can be found in https://github.com/MakieOrg/Makie.jl/tree/master/src/basic_recipes

Inspired by #4302

tuncbkose commented 1 week ago

Is that to-do list accurate/up-to-date? I'm not too familiar with Makie/Julia, which is why I'm in a good first issue, but I could only find 2 untracked instances of those functions: https://github.com/MakieOrg/Makie.jl/blob/86177428baa835d88ee733d4be57b5340de70053/src/basic_recipes/hvspan.jl#L49 and https://github.com/MakieOrg/Makie.jl/blob/86177428baa835d88ee733d4be57b5340de70053/src/basic_recipes/timeseries.jl#L43

The rest seem already updated, for example with commit dafd4d3fa5da8f1bf354571fec52ccbd7bea078b in https://github.com/MakieOrg/Makie.jl/pull/2731.

asinghvi17 commented 1 week ago

Welcome to the community!

Thanks for catching that - it looks like contourf and tricontourf are both OK, I probably included them mistakenly. Series also looks fine.

You mentioned hvspan and timeseries already, but there are a few more:

Volumeslices

https://github.com/MakieOrg/Makie.jl/blob/86177428baa835d88ee733d4be57b5340de70053/src/basic_recipes/volumeslices.jl#L27 (this is a bit tricky, it looks like a regular map but x, y, z are observables. Feel free to change the function invocation to lift for clarity)

Scatterlines has quite a few, the below statement and friends

https://github.com/MakieOrg/Makie.jl/blob/86177428baa835d88ee733d4be57b5340de70053/src/basic_recipes/scatterlines.jl#L39

simonsteiger commented 1 week ago

I'd be interested in helping out as someone with little experience in Makie's internals.

I recall looking at the sankey recipe a few weeks ago and seeing the lift function in there, too. I know that this recipe is not integrated with Makie yet and might be of low priority, but I have to plot sankeys too often and like Makie's layouting too much to not have a try.

Would it be correct to pass lift(plot, x, y, z) there, too?