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

A little experiment towards better clean up + more typed observables #2372

Closed
wants to merge 3 commits into from

Conversation

SimonDanisch
Copy link
Member

I rediscovered while looking at the GLMakie backend code, that we actually are already pretty strict about atomic plot attributes, in the sense that the output type of convert_attributes is not allowed to change. So, e.g. if we set color to a single color, we may never change it to an array of colors, alowing to restrict color to Observable{RGBAf}.

So, at least for atomic plots, we should be able to always create strictly typed observables of the type typeof(conver_arguments(...)).
In this PR, I'm rewriting Attributes to lazily convert via convert_attributes to always return a typed observable...
Still needs to be optional though since Attributes are used for so many things, but with a bit larger refactor, we may be able to flip the switch on that.
Still an experiment, but may greatly help with not doing conversions multiple times, have better typed Observables and have better ways to completely clean up the observable connections of a plot.
If this works out well, we may want to make our own Attribute type for atomic plots (which then can also finally lock in the set of allowed attributes), and use a simpler type for the rest of the Attribute uses.

@jkrumbiegel
Copy link
Member

jkrumbiegel commented Oct 26, 2022

But is it good that you can't change color = rand(10) to color = :red interactively? Or am I misunderstanding what So, e.g. if we set color to a single color, we may never change it to an array of colors, alowing to restrict color to Observable{RGBAf} means

@SimonDanisch
Copy link
Member Author

But is it good that you can't change color = rand(10) to color = :red interactively?

You can't do that right now 🤷

@MakieBot
Copy link
Collaborator

MakieBot commented Oct 26, 2022

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 31.64s (31.48, 32.06) 0.20+- 19.26s (19.01, 19.46) 0.14+- 15.26s (15.12, 15.51) 0.14+- 18.96ms (18.80, 19.06) 0.11+- 51.94ms (51.44, 53.03) 0.55+-
master 32.30s (32.14, 32.56) 0.15+- 18.87s (18.65, 19.13) 0.16+- 16.88s (16.77, 17.07) 0.10+- 18.21ms (17.95, 18.75) 0.27+- 51.70ms (51.19, 52.38) 0.39+-
evaluation -2.11%, -0.67s faster ✓ (-3.75d, 0.00p, 0.18std) +2.04%, 0.39s slower X (2.59d, 0.00p, 0.15std) -10.66%, -1.63s faster✅ (-13.33d, 0.00p, 0.12std) +3.97%, 0.75ms slower X (3.73d, 0.00p, 0.19std) +0.45%, 0.23ms invariant (0.48d, 0.39p, 0.47std)
CairoMakie 32.88s (32.45, 33.60) 0.43+- 23.58s (23.39, 23.87) 0.19+- 3.33s (3.27, 3.39) 0.05+- 25.76ms (25.09, 27.40) 0.78+- 29.18ms (28.21, 29.87) 0.57+-
master 33.64s (33.02, 34.67) 0.55+- 23.21s (23.02, 23.43) 0.15+- 3.29s (3.25, 3.32) 0.03+- 23.54ms (22.68, 24.08) 0.46+- 29.45ms (28.30, 33.95) 2.00+-
evaluation -2.34%, -0.77s faster ✓ (-1.56d, 0.01p, 0.49std) +1.57%, 0.37s slower X (2.15d, 0.00p, 0.17std) +1.31%, 0.04s invariant (1.15d, 0.06p, 0.04std) +8.62%, 2.22ms slower❌ (3.46d, 0.00p, 0.62std) -0.92%, -0.27ms invariant (-0.18d, 0.74p, 1.29std)
WGLMakie
master
evaluation

@SimonDanisch SimonDanisch mentioned this pull request Oct 26, 2022
@ffreyer
Copy link
Collaborator

ffreyer commented Oct 26, 2022

To give some context:

{{uv_offset_width_type}} uv_offset_width;
//{{uv_x_type}} uv_width;
{{position_type}} position;

lines like these get replaced before shader compilation with something like type_of_attribute name_of_attribute;. So if the renderobject gets generated with an RGBAf color, you'd get something like vec4 color; there. After shader compilation you can change the value but not the type.
Making this dynamic would either require converting to arrays beforehand or getting shaders to recompile on type changes.

@SimonDanisch
Copy link
Member Author

Yeah, and WGLMakie & GLMakie at least both have a version of:
https://github.com/MakieOrg/Makie.jl/blob/master/GLMakie/src/drawing_primitives.jl#L163

function lift_convert_inner(value, key, plot_key, plot)
    return lift(value) do value
        return convert_attribute(value, key, plot_key)
    end
end

which is pretty much identical to:

obs = Observable(convert_attribute(value, key, plot_key)) # creates strongly typed obs
map!(x-> convert_attribute(x, ...), obs)

@SimonDanisch SimonDanisch marked this pull request as draft October 26, 2022 13:36
@SimonDanisch
Copy link
Member Author

Closing this experiment for now

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

Successfully merging this pull request may close these issues.

None yet

4 participants