[WIP] Refactor Plot type & road to 1.0 #1085

SimonDanisch · 2021-06-30T17:52:42Z

This is a pretty big rewrite of the internal plot type.
The goal to not be breaking, though! This is a stretch goal, and quite a few things will likely be slightly breaking. One should think of this PR as Makie's 0.7 Julia version, on it's way to a breaking 1.0 release. This refactor will introduce warnings for features we want to deprecate, and will lay the groundwork to do the actual breaking changes.

The planned changes:

each atomic plot type now has a fixed set of fields with fixed types. This enables:
- good error messages for wrong plot attributes (still need to add that bit in the current implementation)
- compilation time improvements (!?)
- types become self documenting
- better API for backends. Right now, it's pretty fuzzy to see what the basic plot types support, leading to bugs + making it more difficult to write new backends/maintain them
- merging themes can be a clean operation. Right now it's a big mess where it happens, how and when. With the new plot type, all fields not set by a user are put into a set of uninitilized_fields, and then get updated from the theme when actually drawn
make it possible to construct a plot type without a scene. This enables
- constructing plots, taking their boundingbox etc, without plotting anything
- easier and cleaner adding & removing of plots from scenes. Right now it's possible, but unclean since the plot type still keeps some connections to the original parent
- reactive API: one can build whole scene graphs of plots without the need of a parent scene, and can construct them cheaply in an animation loop, without the whole overhead of building the whole scene. This also makes diffing of scene graphs easier (also because of the above)
revamp of observables inside plot types
- plot types are now mutable with concrete values as fields. Any setfield! operation will immediately converted the values to the strict field types
- remove the Attributes type, which was storing any plot attribute as Observable{Any}
- each plot object has an on_event observable, which one can use to register to setfield! events, or other events like mouse/camera/axis-limit-changes
- non observable workflows become first class, since the creation of a plot type without any observable doesn't create any new observables, while one can still subscribe to attributes updating via setfield!
- no duplicate convert_attribute calls in multiple parts of the pipelines. In the old infrastructure, everything got converted lazily, so there wasn't a clear point to convert.. Some some recipe did already apply conversions, and the backend then applied conversions again
- enables updating multiple attributes together, fixing the age old problems with synchronous updates. Related API suggestion for avoiding synchronous update issues #798
conversion pipeline in plot constructor
- clear point where the conversion happens
- Enables to have e.g. Scatter(1:4) to create the correct plot object, without a scene present
- makes it possible to create plot instances very early, making it easier in the rest of the pipeline to e.g. calculate boundingboxes earlier

Timeline:

timholy

Here's a preliminary review focusing just on your struct types. I am not sure I'll be able to provide much more useful feedback at this stage, but hopefully this helps at least somewhat.

Anything user-facing might need to stay generic, but if you're expecting a fair amount of transformation anyway (e.g., in terms of Float32 argtypes or something), then you might as well transform the container in addition to the eltype. For reference, here are the rules:

julia> isconcretetype(Vector{Float32})
true

julia> isconcretetype(AbstractVector{Float32})
false

julia> isconcretetype(Vector{Union{Nothing,Float32}})
true

julia> isconcretetype(Vector{Any})  # *elements* are not concretely-typed, but the *container* is
true

julia> isconcretetype(Vector{<:Real})
false

Declaring something like

struct Scatter{N,A<:AbstractVector}
    somefield::A
    ...

is of course fine from the standpoint of inference of somefield (it will be concrete if the concrete Scatter is known), but problematic from the standpoint of more type-diversity among Scatter concrete types and thus longer latency.

There are cases where your AbstractVector field is the correct solution, but you should make sure you're very clear on why you're doing it that way. Basically that comes down to circumstances where you deliberately want to "hide the type" as a strategy to reduce latency. Under such circumstances you'd have carefully designed function-barriers that trigger runtime specialization on the specific AbstractVector. When deliberate (and in conjunction with some carefully-placed @nospecialize annotations), this can be an effective and powerful strategy, but it's not something you should do by accident.

timholy · 2021-08-23T15:39:16Z

MakieCore/src/basic_plots/lines.jl

@@ -0,0 +1,43 @@
+@plottype mutable struct Lines{N} <: AbstractPlot{Any}
+    position::AbstractVector{Point{N,Float32}}


Does this have to be an AbstractVector? There are sometimes reason to choose that but I'm sure you know it will be non-inferrable. Since you're specifying the eltype so tightly, it seems likely this won't be user-facing anyway, and so can you just make this Vector?

Yeah, that's up for debate... I would really like to keep it an AbstractVector, since there are quite a few use cases like efficiently mapping a view to a gpu buffer, or passing through GPUArrays and things like that...
But I guess we should benchmark the impact, and if it's huge, maybe try to stick to Vector as much as possible...

Would it help to make the type of positions field a type-parameter of Lines?

You can keep it AbstractVector. The trick is either (1) to not pull it out of the Lines struct until the "last minute" and/or (2) annotate dispatches @nospecialize(position::AbstractVector{...}) until you deliberately want a function barrier (basically, for any method with a loop over the elements of position).

But the N type parameter will break @nospecialize, i.e., you can't expect foo(@nospecialize(position::AbstractVector{Point{N,Float32}})) where N to not fully specialize on position.

If your diversity of position types is not large, then this may not be a big problem. It sounds like you're going to become a @nospecialize expert soon 🙂 .

Would it help to make the type of positions field a type-parameter of Lines?

It would improve inferrability at the cost of likely greater latency, because the compiler would have to specialize every call for every Scatter type you use.

timholy · 2021-08-23T15:57:59Z

MakieCore/src/basic_plots/scatter.jl

@@ -0,0 +1,36 @@
+const MarkerSizeTypes{N} = TorVector{Union{Float32, Vec{N, Float32}}}


It's remotely possible that it will be better to "flatten" the Union here, but let's wait and see how the inference works out (it's identical in consequence, so that's trivial to change later).

timholy · 2021-08-23T15:58:49Z

MakieCore/src/basic_plots/scatter.jl

@@ -0,0 +1,36 @@
+const MarkerSizeTypes{N} = TorVector{Union{Float32, Vec{N, Float32}}}
+const MarkerTypes = TorVector{<:Union{Symbol,Char, Type{Circle}, Type{Rect2D}}}
+const DistanceFieldType = Union{Nothing, AbstractMatrix{<: Union{Colorant, Number}}}


The more concrete you can make this, the better for inference & latency. But anything user-facing probably has to be general. You could always use

const DistanceFieldType = Union{Nothing, Matrix{<: Union{Colorant, Number}}}

for struct fields and

const DistanceFieldTypeLike = Union{Nothing, <:AbstractMatrix{<: Union{Colorant, Number}}}

for dispatch to indicate "something that could obviously be converted to a DistanceFieldType.

timholy · 2021-08-23T16:03:30Z

MakieCore/src/types.jl

@@ -68,6 +64,12 @@ end

 const px = Pixel(1)

+const Space = Type{<: Unit}


This will be rough for some of your previous field types...if it's possible to standardize internally...

Yeah, I think this was just a stop gap solution, to not trigger a bigger refactor... I think this can just be an enum going forward..

timholy · 2021-08-23T16:05:24Z

MakieCore/src/basic_plots/text.jl

+    font::NativeFont = "Dejavue Sans"
+    strokecolor::ColorType = (:black, 0.0)
+    strokewidth::TorVector{Float32} = 0
+    align::Vec2f0 = (:left, :bottom)


Hmm, how does this work? I can't imagine a method that doesn't involve type-piracy. Oh, unless the @plottype handles it.

timholy · 2021-08-23T16:08:27Z

MakieCore/src/types.jl

-abstract type AbstractScreen <: AbstractDisplay end
-
-const SceneLike = Union{AbstractScene, ScenePlot}
+const TorVector{T} = Union{T, AbstractVector{T}}


Since this is used in struct fields, it sure would be nice to make this Vector{T} instead of AbstractVector if possible.

Yaaah... 😢 I planned to Benchmark this when the PR is in a better state - but would be sad to give up on AbstractArray support, but maybe we can make it work one way or the other...

You can of course have both TorAbstractVector and TorVector. If nothing else you might want to change the name to acknowledge the abstraction. (TorAVector would be pretty short if that's an issue.)

SimonDanisch · 2022-03-23T13:46:49Z

Will close this for now... Will bring back the important changes by cherry picking in much smaller PRs!

SimonDanisch added 12 commits June 29, 2021 21:08

start plots type refactor

74c54c7

move things around

48bbb2b

Merge branch 'master' into sd/refactor

f48d4ce

implement scatter

d187c0b

implement updates +obs

16ebcae

merge master

9ac2c93

make scatter not error

a1831eb

Merge branch 'master' into sd/refactor

f343032

make scatter work

f72356b

add more types

89d00da

make more stuff work

5d3287c

make more plots work

14dec1e

SimonDanisch changed the title ~~[WIP] Refactor Plot type~~ [WIP] Refactor Plot type & road to 1.0 Jul 15, 2021

merge master

64056c1

SimonDanisch mentioned this pull request Jul 26, 2021

Transition to Plots.jl #8

Closed

merge master

a1349d4

timholy mentioned this pull request Aug 17, 2021

Reducing latency: acceptable strategies discussion #792

Closed

henry2004y mentioned this pull request Aug 19, 2021

Makie plot recipe henry2004y/Vlasiator.jl#22

Closed

3 tasks

timholy reviewed Aug 23, 2021

View reviewed changes

SimonDanisch mentioned this pull request Mar 22, 2022

Change behaviour of getindex/getproperty for plotobjects #1779

Closed

SimonDanisch closed this Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Refactor Plot type & road to 1.0 #1085

[WIP] Refactor Plot type & road to 1.0 #1085

SimonDanisch commented Jun 30, 2021 •

edited

Loading

timholy left a comment •

edited

Loading

timholy Aug 23, 2021

SimonDanisch Aug 23, 2021

diegozea Aug 23, 2021 •

edited

Loading

timholy Aug 23, 2021 •

edited

Loading

timholy Aug 23, 2021

timholy Aug 23, 2021

timholy Aug 23, 2021

timholy Aug 23, 2021

SimonDanisch Aug 23, 2021

timholy Aug 23, 2021 •

edited

Loading

timholy Aug 23, 2021

SimonDanisch Aug 23, 2021

timholy Aug 23, 2021 •

edited

Loading

SimonDanisch commented Mar 23, 2022

		@@ -0,0 +1,43 @@
		@plottype mutable struct Lines{N} <: AbstractPlot{Any}
		position::AbstractVector{Point{N,Float32}}

		@@ -0,0 +1,36 @@
		const MarkerSizeTypes{N} = TorVector{Union{Float32, Vec{N, Float32}}}

		@@ -68,6 +64,12 @@ end

		const px = Pixel(1)

		const Space = Type{<: Unit}

[WIP] Refactor Plot type & road to 1.0 #1085

[WIP] Refactor Plot type & road to 1.0 #1085

Conversation

SimonDanisch commented Jun 30, 2021 • edited Loading

timholy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegozea Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

timholy Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

SimonDanisch commented Mar 23, 2022

SimonDanisch commented Jun 30, 2021 •

edited

Loading

timholy left a comment •

edited

Loading

diegozea Aug 23, 2021 •

edited

Loading

timholy Aug 23, 2021 •

edited

Loading

timholy Aug 23, 2021 •

edited

Loading

timholy Aug 23, 2021 •

edited

Loading