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

Dynamic PlotList recipe which plots arbitrary arrays of PlotSpec #2868

Merged
merged 12 commits into from
Sep 27, 2023

Conversation

asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented Apr 18, 2023

Description

As discussed in MakieCon 2023 Day 1...will add an example in a bit.

This currently takes the form of a recipe, but we could make it a normal function which is just passed an Observable list of PlotSpec.

fig = Figure()
ax = Axis(fig[1, 1])

pl = plotlist!(
    ax,
    [PlotSpec{Lines}(0..1, sin.(0:0.01:1); color = :blue), PlotSpec{Heatmap}(0..1, 0..1, Makie.peaks(); transformation = (; translation = Vec3f(0, 0, -1)))]
)

fig

plot_1029

pl[1][] = [PlotSpec{Lines}(0..1, sin.(0:0.01:1); color = :red), PlotSpec{Heatmap}(0..1, 0..1, Makie.peaks(); transformation = (; translation = Vec3f(0, 0, -1)))]

fig

plot_1030

pl[1][] = [
    PlotSpec{Lines}(0..1, sin.(0:0.01:1); color = Makie.resample_cmap(:viridis, 101)), 
    PlotSpec{Surface}(0..1, 0..1, Makie.peaks(); transformation = (; translation = Vec3f(0, 0, -1))),
    PlotSpec{Poly}(Rect2f(0.45, 0.45, 0.1, 0.1)),
]

fig

plot_1031

pl[1][] = [
    PlotSpec{Surface}(0..1, 0..1, Makie.peaks(); colormap = :viridis, transformation = (; translation = Vec3f(0, 0, -1))),
]

fig

plot_1032

Type of change

Delete options that do not apply:

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@asinghvi17
Copy link
Member Author

This is functionally complete, but please try whatever you think might break it and let me know!

I think we might also want to add eg palette support but that would need a refactor of the axis code.

@asinghvi17 asinghvi17 marked this pull request as ready for review April 18, 2023 22:48
@MakieBot
Copy link
Collaborator

MakieBot commented Apr 18, 2023

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 13.64s (13.36, 13.95) 0.20+- 1.00s (0.96, 1.03) 0.02+- 1.44s (1.39, 1.52) 0.05+- 12.29ms (12.08, 12.77) 0.28+- 167.87ms (165.04, 170.30) 1.86+-
master 13.32s (13.05, 13.55) 0.18+- 1.08s (1.06, 1.09) 0.01+- 1.44s (1.42, 1.47) 0.02+- 12.10ms (11.84, 12.55) 0.23+- 167.01ms (164.43, 169.75) 2.17+-
evaluation +2.32%, 0.32s slower X (1.63d, 0.01p, 0.19std) -7.93%, -0.08s faster✅ (-4.75d, 0.00p, 0.02std) -0.05%, -0.0s invariant (-0.02d, 0.97p, 0.03std) +1.56%, 0.19ms invariant (0.75d, 0.19p, 0.25std) +0.51%, 0.86ms invariant (0.43d, 0.44p, 2.01std)
CairoMakie 10.43s (10.32, 10.60) 0.10+- 855.66ms (837.47, 890.28) 17.99+- 572.31ms (564.51, 586.59) 7.46+- 10.36ms (10.25, 10.52) 0.11+- 5.43ms (5.37, 5.47) 0.04+-
master 10.24s (10.15, 10.37) 0.07+- 911.13ms (890.73, 931.88) 12.87+- 539.36ms (529.19, 547.91) 5.93+- 10.20ms (10.03, 10.36) 0.13+- 6.05ms (5.97, 6.18) 0.08+-
evaluation +1.84%, 0.19s slower X (2.18d, 0.00p, 0.09std) -6.48%, -55.48ms faster✅ (-3.55d, 0.00p, 15.43std) +5.76%, 32.95ms slower❌ (4.89d, 0.00p, 6.69std) +1.55%, 0.16ms slower X (1.38d, 0.02p, 0.12std) -11.50%, -0.62ms faster✅ (-10.07d, 0.00p, 0.06std)
WGLMakie 13.82s (13.59, 14.17) 0.18+- 1.08s (1.04, 1.10) 0.02+- 12.62s (12.36, 13.02) 0.23+- 17.22ms (15.30, 24.77) 3.35+- 1006.94ms (953.10, 1062.52) 37.73+-
master 13.55s (13.44, 13.68) 0.08+- 1.14s (1.06, 1.20) 0.05+- 12.66s (12.41, 12.95) 0.19+- 15.33ms (14.44, 16.28) 0.64+- 998.94ms (948.45, 1064.92) 41.76+-
evaluation +1.99%, 0.28s slower X (1.96d, 0.01p, 0.13std) -5.53%, -0.06s faster✅ (-1.42d, 0.03p, 0.04std) -0.36%, -0.05s invariant (-0.22d, 0.69p, 0.21std) +10.98%, 1.89ms noisy🤷‍♀️ (0.78d, 0.19p, 1.99std) +0.79%, 8.0ms invariant (0.20d, 0.71p, 39.74std)

@SimonDanisch SimonDanisch mentioned this pull request May 23, 2023
@pepijndevos
Copy link
Contributor

This currently takes the form of a recipe, but we could make it a normal function which is just passed an Observable list of PlotSpec.

I tried this out and immediately ran into problems where things like color cycling don't work because it's a recipe. The below example produces only black lines, instead of cycling through colors if you'd call lines! twice.

plotlist([PlotSpec{Lines}(0:5, 0:5), PlotSpec{Lines}(0:5, ones(6))])
image

@pepijndevos
Copy link
Contributor

I tried this some more, because I really need this functionality.

First I tried to make it not a recipe, but got weird errors.

Then I tried just using the recipe with explicit attributes, which "works" in that it doesn't error, but rather leaves dangling plot elements (that don't resolve themselves if you change back the Observed) so once you remove a plot it's just broken.

It seems there was also an off-by-one error in length(old_plotspec_types):-1:(length(plotspec_types)+1) on line 64

My recipe is something like

function Makie.plot!(plt::SignalPlot{<:Tuple{<:AbstractArray}})
    Main.plt = plt
    ms = plt[1]
    specs = lift(ms, values(plt.attributes)...) do ms, attrs...
        pairs = zip(keys(plt.attributes), attrs)
        [PlotSpec{SignalPlot}(item; pairs...) for item in ms]
    end
    plotlist!(plt, specs)
end

@staticfloat
Copy link
Contributor

staticfloat commented Jul 7, 2023

I tried re-working the fundamental machinery in here to use a Dict instead of an array, so that we don't have to track order of plots (since as far as I can tell, order doesn't matter):

function update_plot_with_plotspec!(cached_plot::AbstractPlot, spec::PlotSpec)
    # Update args in cached `input_args` list
    for arg_index in eachindex(spec.args)
        cached_plot.input_args[arg_index].val = spec.args[arg_index]
    end
    # Update attributes
    for (attribute, new_value) in pairs(spec.kwargs)
        update_attributes_inplace!(cached_plot, attribute, new_value)
    end
    
    # notify all args, without broadcasting
    map(notify, cached_plot.input_args)
    # notify all accessed kwargs
    map(_notify!, (getindex(cached_plot, attr) for attr in keys(spec.kwargs)))
end

function Makie.plot!(p::PlotList{<: Tuple{<: AbstractArray{<: PlotSpec}}})
    # Cache plots here so that we aren't re-creating plots every time;
    # if a plot still exists from last time, update it accordingly.
    # If the plot is removed from `plotspecs`, we'll delete it from here
    # and re-create it if it ever returns.
    cached_plots = IdDict{Any,AbstractPlot}()

    lift(p[1]) do plotspecs
        Main.p = p
        println()
        println("----------------------------------------")
        plots_to_delete = Base.IdSet()
        push!.((plots_to_delete,), collect(keys(cached_plots))))

        # First, scan through `plotspecs`, creating or updating all plots
        # specified by `plotspecs`, marking previously-extant plots as not
        # to be deleted via `plots_to_delete`
        for plotspec in plotspecs
            if !haskey(cached_plots, plotspec.args[1])
                # Create new plot, store it into our `cached_plots` dictionary
                cached_plots[plotspec] = plot!(p,
                    typeof(plotspec).parameters[1],
                    Attributes(plotspec.kwargs),
                    plotspec.args...,
                )
            else
                update_plot_with_plotspec!(cached_plots[plotspec], plotspec)
            end

            # Because this plotspec appeared in `plotspecs`, it should not
            # be deleted, so remove it from our deletion queue.
            delete!(plots_to_delete, plotspec)
        end

        # Next, delete all plots still extant in in `plots_to_delete`
        for plotspec in plots_to_delete
            idx = findfirst(==(cached_plots[plotspec]), p.plots)
            if idx !== nothing
                deleteat!(p.plots, idx)
            end
            delete!(cached_plots, plotspec)
        end
    end
end

But for some reason I can't figure out, updates don't flow through the system; my plots remain static.

@SimonDanisch
Copy link
Member

While reviewing & figuring out your approaches, I've rewritten the implementation.
Caching needs to be based on the argument + attribute types of the plot spec, because that's what needs to be the same to be "updatable". Also, I now plot directly into the scene and remove plots from that scene, because if we add the plots nested inside the recipe plot type, it gets a bit more complicated to delete!.

I also simplified the updating mechanism for now, removing nested attributes, which aren't that well supported within Makie yet anyways and shouldn't be needed for most plots.
The transformation argument is maybe the biggest exception, but that seems to be broken right now?

Anyways, I also played around with a simple API to make it a bit nicer to use:

import Makie.PlotspecApi as P
plots[] = [
    P.image(0 .. 1, 0 .. 1, Makie.peaks()),
    P.poly(Rect2f(0.45, 0.45, 0.1, 0.1)),
    P.lines(0 .. 1, sin.(0:0.01:1); linewidth=10, color=Makie.resample_cmap(:viridis, 101)),
]

@SimonDanisch
Copy link
Member

SimonDanisch commented Jul 10, 2023

@pepijndevos not sure what to do about the cycling, since that needs the axis.
I've been wanting to make cycling part of the scene, which should fix this... Otherwise, I guess we need to manually re-implement the cycling inside the recipe, not sure how to do this correctly otherwise.
edit: I mean correctly inside a recipe... Of course as you suggested we could directly overload plot!(ax, PlotSpec[...]), which maybe isn't such a bad idea, considering it already directly plots into the scene unlike a "reasonable" recipe.

@staticfloat
Copy link
Contributor

Yes, I agree. The PlotList functionality feels much more "top-level" than most recipes; I would expect other recipes to fit inside of it, so having the PlotList plotting directly interact with axes makes sense to me.

@SimonDanisch SimonDanisch changed the base branch from master to sd/beta-20 September 26, 2023 15:10
@SimonDanisch
Copy link
Member

This isn't 100% ready, but since sd/beta-20 had some problems with the PlotSpec implementation which are cleaned up in here, I'm going to merge this and polish the rest in sd/beta-20.
For sd/beta-20, the remaining work is also just polish and testing, so I might just do it in one go.

@SimonDanisch SimonDanisch merged commit 98da09b into sd/beta-20 Sep 27, 2023
7 of 13 checks passed
@SimonDanisch SimonDanisch deleted the as/plotlist branch September 27, 2023 15:50
@SimonDanisch SimonDanisch mentioned this pull request Oct 6, 2023
@SimonDanisch SimonDanisch changed the title [WIP] Dynamic PlotList recipe which plots arbitrary arrays of PlotSpec Dynamic PlotList recipe which plots arbitrary arrays of PlotSpec Oct 6, 2023
@ffreyer ffreyer mentioned this pull request Oct 28, 2023
16 tasks
SimonDanisch added a commit that referenced this pull request Nov 17, 2023
Continues #2831 !
Still needs to check, if I rebased correctly and didn't incorrectly
apply some of the changes!

## Merged PRs
- #2598
- #2746
- #2346
- #2544
- #3082
- #2868
- #3062
- #3106
- #3281
- #3246

## TODOS

- [x] fix flaky test `@test GLMakie.window_size(screen.glscreen) ==
scaled(screen, (W, H))`
- [x] Merge axis type inferences from #2220 
- [x] Test on different resolution screens, IJulia, Pluto, VSCode,
Windowed
- [x] rebase to only have merge commits from the PRs 
- [x] investigate unexpected speed ups
- [x] reset camera settings from tests
- [ ] check doc image generation
- [x] rethink default near/far in Camera3D (compatability with OIT)
- [x] merge #3246
- [x] fix WGLMakie issues/tests:
- [x] fix line depth issues (see tests: ~~hexbin colorrange~~ (not new),
LaTeXStrings in Axis3, Axis3 axis reversal)
  - [x] fix lighting of surface with nan points (fixed in #3246)
- ~~volume/3D contour artifacts (see 3D Contour with 2D contour
slices)~~ not new
  - ~~artifacting in "colorscale (lines)"~~ not new
- [x] GLMakie:
  - [x] slight outline in "scatter image markers" test
  - ~~clipping/z-fighting in "volume translated"~~ not new
- [x] CairoMakie:
  -  ~~Artfiacting in `colorscale (lines)"~~ not new
  - ~~markersize in "scatter rotations" changed?~~ not new
  - ~~color change in "colorscale (poly)"~~ not new
  - ~~transparency/render order of "OldAxis + Surface"~~ not new
  - ~~render order in "Merged color mesh"~~ not new
  - ~~render order of "Surface + wireframe + contour"~~ not new
- [x] Check "SpecApi in convert_arguments" (colors swapped?)


## Fixes the following errors

- fixes #2721 via #2746
- fixes #1600 via #2746
- fixes #1236 via #2746
- fixes MakieOrg/GeoMakie.jl#133 via #2598
- closes #2522
- closes #3239 via #3246
- fixes #3238 via #3246
- fixes #2985 via #3246
- fixes #3307 via #3281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants