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

[WIP] Using a Compute graph as alternative to Observables #4630

Open
wants to merge 111 commits into
base: master
Choose a base branch
from

Conversation

SimonDanisch
Copy link
Member

@SimonDanisch SimonDanisch commented Nov 26, 2024

This introcudes ComputePipeline (all names very WIP) as an alternative to Observables, which solves:

  • some performance problems
  • multi input/output situations
  • updating multiple attributes at once
  • thread safety and better clean up
  • Hopefully better stacktraces with clearer input/output mappings
  • Maybe better compile times and inference

State of this PR: first implementation for Scatter, hacking into the plot pipeline to replace the Scatter constructor and hijack it with a computegraph implementation for convert_arguments/convert_attributes and any backend computation.
Only prototyped for GLMakie right now.
It currently leaves the plot type unchanged and stores the compute graph in plot.args[1] (lol) and overloads a few methods already just for Scatter (not getproperty etc though, yet).

Next steps:

  • axis dim converts
  • get/setproperty
  • more benchmarks
  • WGLMakie/CairoMakie test
  • Same treatment for lineplot, which should really help see any performance gains

Continuation of: #4550 and #4360

Comment on lines 239 to 244
if isnothing(value)
edge.outputs_dirty[i] = false
else
edge.outputs_dirty[i] = true
edge.outputs[i][] = value
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly this interprets nothing as "the value has not changed". In that case shouldn't it leave the dirty state untouched? If a previous call has dirtied output i and this call has not changed it, it should still dirty, right?

Suggested change
if isnothing(value)
edge.outputs_dirty[i] = false
else
edge.outputs_dirty[i] = true
edge.outputs[i][] = value
end
if !isnothing(value)
edge.outputs_dirty[i] = true
edge.outputs[i][] = value
end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case shouldn't it leave the dirty state untouched? If a previous call has dirtied output i and this call has not changed it, it should still dirty, right?

No, the dirty state is local to the current resolve.
So needs to be set to the correct value when setting the new value.
We still need to double check if there's a simpler and more consistent way and add some tests if this works 100% as intended, but right now outputs_dirty is only for the children of an edge to figure out after a resolve if a value has changed from that particular resolve.

@MakieBot
Copy link
Collaborator

MakieBot commented Dec 16, 2024

Benchmark Results

SHA: b972279598d735ea8fd73453b3855b3e38cb2498

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

@@ -176,7 +176,7 @@ is called. Note that resolution must be smaller than `1 / eps(Float32)`.
"""
function Float32Convert(resolution = 1e4)
scaling = LinearScaling(Vec{3, Float64}(1.0), Vec{3, Float64}(0.0))
return Float32Convert(Observable(scaling), resolution)
return Float32Convert(Observable(scaling; ignore_equal_values=true), resolution)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be useless because update_limits!() already skips updates when the current scaling is acceptable.

asinghvi17 and others added 5 commits January 21, 2025 13:38
…omputePipeline package (#4656)

* Update ci.yml

* Update reference_tests.yml

* Update ci.yml

* Update reference_tests.yml

* Update ci.yml

* Update reference_tests.yml

---------

Co-authored-by: Simon <[email protected]>
@ffreyer
Copy link
Collaborator

ffreyer commented Jan 25, 2025

Just a thought related to #4752 - would it make sense to have "versioned" nodes in the compute graph?

For example with linesegments we have color -> scaled_color -> synched_color. What if instead they were all just named "color" and internally treated as different nodes acting as versions of the name. When requesting "color" you'd get the latest version of it, so that in the backend the detail of what the Makie does disappears. This might be as simple as making graph.outputs[name] and array where each index is a new version.

This would make the pipeline easier to adjust because naming would become easier. E.g. if I wanted to hack the linesegments pipeline to do something to synched_color before rendering, I would have to rename that node and add a computation creating a new "synched_color". With versions it you could just add another computation from color (V3) to color (V4) and you'd be done.

That would be quite useful for transform functions that change geometry as described in #4752. Maybe it would also be useful in recipes?

Comment on lines +488 to +489
attribute_per_pos!(attr, :scaled_color, :synched_color)
attribute_per_pos!(attr, :linewidth, :synched_linewidth)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only applies to LineSegments. For them it maps a value per point pair to two values for the vertices, which results in per-segment colors/linewidths. In Lines this would result in a const value segment, then a gradient segment, then a const value segment, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

4 participants