Diamond dependency patterns can cause issues with @lift

[staticfloat]

• are you running newest version (version from docs) ?
• can you reproduce the bug with a fresh environment ? (]activate --temp; add Makie) #3659

It appears that if I use @lift() to make a recipe dynamic, I can quickly run into issues where one half of my observables have updated, but not both. Example:

using WGLMakie

@recipe(RecipePlot) do scene
    Attributes(
        color = theme(scene, :linecolor),
    )
end

struct Timeseries
    xs
    ys
end

function Makie.plot!(plt::RecipePlot; kwargs...)
    # This creates a diamond dependency, and one of these
    # `xs` or `ys` will get updated without the other.
    xs = @lift $(plt[1]).xs
    ys = @lift $(plt[1]).ys
    scatter!(plt, xs, ys)
end

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

    sg = SliderGrid(fig[2,1], (label = "signal", range=1:2, startvalue=2))

    sigs = [
        Timeseries(1:10, 1:10),
        Timeseries(1:20, 1:20),
    ]

    sliderobservable = only(sg.sliders).value
    sig = @lift($(sigs)[$(sliderobservable)])
    recipeplot!(ax, sig)

    fig
end

Running this and moving the slider results in the error:

DimensionMismatch: arrays could not be broadcast to a common size: a has axes Base.OneTo(10) and b has axes Base.OneTo(20)

If I change the recipe to instead do a single conversion to Point2f I can fix this:

function Makie.plot!(plt::RecipePlot; kwargs...)
    points = @lift(WGLMakie.Point2f.($(plt[1]).xs, $(plt[1]).ys))
    scatter!(plt, points)
end

However this is not obvious from the error message, and IMO is a bit of a footgun. Reading the docs, I see there is a paragraph at the very end of the Problems with Synchronous Updates section that seems related, only I argue this behavior is even more surprising because from the user's perspective it feels like a single observable goes in.

[jkrumbiegel]

It's totally a footgun, I agree. It's quite hard to get around though, because the update of an observable can cause arbitrarily complex behavior. So if you want to update two of them, they can have effects that can be consolidated or not. I've not been able to come up with a general solution to this problem, yet. We work around some cases with updating .val but that also doesn't always work.

What's not good is that to do the .val strategy correctly, you have to know what the effects of the observables are, which for the general case, a user cannot know (without reading all the source code). As far as I know, in the beginning Makie used some other signalling pattern, and switched to eager Observables later because it was simpler in some aspects. Maybe @SimonDanisch can give more context on that.

[SimonDanisch]

I've been thinking about this more recently, since the problem is especially visible in the line updating for WGLMakie, where you update positions and an array of colors.
I think after #3650, this should probably become a priority.
My current plan is to refactor the Plot object internals, to allow update!(plot; multiple_attributes...) which then gets correctly implemented in the backends.

[staticfloat]

Yes, I think a batch update solves one half of the problem. The other half is that a function like scatter needs to know if both xs and ys are being updated, and if so, it needs to wait until the update is complete before consuming the values. I propose that when the batch update is started, it first iterates through the network of observables and “locks” them all, without changing their values. Then, it propagates the changes through, and only after a value has been changed is it unlocked, and only after all previously-locked input arguments to a plotting function are unlocked does it consume the values. We could make the lift macro automatically wait until all interpolated observable values are unlocked before evaluating the expression, for instance. This would provide a natural building block for this kind of synchronization primitive.

…

On Tue, Feb 27, 2024 at 01:30 Simon ***@***.***> wrote: I've been thinking about this more recently, since the problem is especially visible in the line updating for WGLMakie. I think after #3650 <#3650>, this should probably become a priority. My current plan is to refactor the Plot object internals, to allow update!(plot; multiple_attributes...) which then gets correctly implemented in the backends. — Reply to this email directly, view it on GitHub <#3658 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA762BSCAHG3AFUZWJM3HDYVWRU5AVCNFSM6AAAAABD3MXJGOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWGEZTGMRQGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ffreyer]

Some other solutions to DimensionMismatch errors we've talked about about before:

• truncate vector observables to the same length internally. This has the issue of hiding data when the input lengths don't match if we don't throw a warning. And if we do we'd be throwing a lot of unnecessary warnings.
• Synchronized updates JuliaGizmos/Observables.jl#109 Iirc this is a more formal and slightly better version of obs.val updates like in the docs.
• https://github.com/jkrumbiegel/PullObservables.jl, i.e. let the backend pull updates rather than the user push updates. This can still trigger errors depending on timings, though we could probably handle these as warnings? It would also likely improve performance by avoiding unnecessary updates. But this is a very big change, which likely necessitates rethinking some of the internal update chains

[jkrumbiegel]

I first liked the PullObservables approach, but it has one big problem. When you "pull" an observable, you rely on knowing which of the observables it depends on have changed, so that you only do as much updating as necessary. However, it's really easy to change values in mutable objects that are just referenced somewhere by these observables. And then you never receive the update because nobody invalidated the downstream observables that were implicitly dependent on those mutable objects.

[ffreyer]

That's true for the current Observables too though?

obs = Observable(zeros(10)) 
obs[][1] = 1

is not going to trigger an update