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 79 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.

@ffreyer
Copy link
Collaborator

ffreyer commented Dec 10, 2024

Progress on GLMakie:

The number of failing tests is now down to 15, with 27 tests throwing errors.

TODOs/Problems:

  • Legend & Colorbar integration
  • dim_converts integration
  • SpecAPI integration
  • dynamic px_per_unit changes (e.g. through save) (this may not work with camera matrix caching as it is implemented here)
  • scatter may not match billboarding behavior of master, as rotation isa Billboard never happens. Needs tests on master to compare
  • axis3d!() sometimes draws wrong, maybe a transformation issue?
  • transform overwrite don't work yet

ffreyer and others added 9 commits December 10, 2024 16:35
* deparametrize Computed and cache outputs_dirty in Computed

* cleanup/fix output_dirty propagation

* remove redundant output_dirty from Input

* fix Input constructor

* rename ComputedValue -> Computed

* skip outputs_dirty (update Computed directly)

* improve printing a bit

---------

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

ffreyer commented Dec 11, 2024

CairoMakie is now at 2 fails - 28 errors, GLMakie at 14-27

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.

3 participants