Skip to content

rangebars: don't show whiskers when not needed #5004

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

rangebars: don't show whiskers when not needed #5004

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/basic_recipes/error_and_rangebars.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ function _plot_bars!(plot, linesegpairs, is_in_y_direction)

scene = parent_scene(plot)

whiskers = lift(plot, linesegpairs, scene.camera.projectionview, plot.model,
whiskers = iszero(whiskerwidth[]) ? nothing : lift(plot, linesegpairs, scene.camera.projectionview, plot.model,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would also mean you can't make whiskers visible after the plot call. I.e. plot.whiskerwidth[] = 5.

Once we're done with #4630 this can be handled more efficiently with the compute graph too. Probably by just making the whisker plot invisible, because that should stop rendering from requesting this calculation to run.

Copy link
Contributor Author

@aplavin aplavin May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This would also mean you can't make whiskers visible after the plot call. I.e. plot.whiskerwidth[] = 5.

Exactly, that's my worry that I indicated as well.

#4630 looks a bit scary in how breaking it can be for all the code using Makie + Observables :)

Still, would be great to have this optimization in Makie – now I'm forced to use my Makie fork in an app to achieve reasonable performance. For rangebars, "no whiskers" is the most common scenario given that it's the default.

Copy link
Member

Choose a reason for hiding this comment

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

yeah this was always a drawback of the observables design, that you pay for the whole graph most of the time, even if parts are unused/hidden. How many rangebars were you using to notice the slowness? You could also use linesegments instead of keeping a fork, that should be little refactoring work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tens of thousands points, and the difference becomes noticeable in interactive usage.
The fork is not just for this PR, but for all my performance optimizations (see several adjacent PRs) that accumulate to 10x-100x improvement in my scenario.

Copy link
Contributor Author

@aplavin aplavin May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Here, it's not really determined at any point that these calculations are not needed, so even adding "pruning" won't help by itself. Whiskers are always calculated and plotted all the way to the end, they just often (including the default) "happen" to have zero length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking a bit more, I don't see any downside with treating whiskerwidth < 0 as "I don't really need whiskers, and won't update whiskerwidth for this plot". This can even remain undocumented, just with a comment in the code to explain the condition.

Is anyone against it? Otherwise I can update this PR correspondingly.

Copy link
Member

Choose a reason for hiding this comment

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

and won't update whiskerwidth for this plot

I don't agree with that, I've tried hard to make as many things in Makie updateable as possible, and regard the places where it's not possible as "design warts". But if the compute graph helps to shut down specific branches if some conditions hold, that would be a very good way of dealing with this.

What could be done before that is to avoid the calculation of the whiskers if their width is zero, for example make all coordinates NaN or some constants. It wouldn't matter that they are in the wrong spot as long as they are not visible anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In #4630 rendering pulls data in. If a primitive plot is not visible, the backend won't bother to pull. So if this stuff gets translated setting the whisker plot to visible = false is equivalent to never triggering this lift. Well, probably once on initialization in W/GLMakie and really never in CairoMakie.

Btw another way you can probably drastically cut the cost of this is to use something like scatter(errorbar_endpoints, marker = Rect, markersize = Vec2f(whiskerwidth, whiskerlinewidth)). That should help with the whisker-visible case too

Copy link
Contributor Author

@aplavin aplavin May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't agree with that, I've tried hard to make as many things in Makie updateable as possible, and regard the places where it's not possible as "design warts".

I would take "no overhead" over "internal design warts" any time, whenever something is performance-sensitive – which it often is for interactive plots :)

It's just unfortunate than one has to depend on my Makie fork for these optimizations, meaning that everyone else using rangebars experience this huge overhead from something they don't need nor use.

Copy link
Member

Choose a reason for hiding this comment

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

well, we do want to merge the compute graph pretty soon, so I guess we might just wait for that before introducing such a workaround?

scene.viewport, transform_func(plot), whiskerwidth) do endpoints, _, _, _, _, whiskerwidth

screenendpoints = plot_to_screen(plot, endpoints)
Expand All @@ -222,7 +222,7 @@ function _plot_bars!(plot, linesegpairs, is_in_y_direction)
return [p for pair in screenendpoints_shifted_pairs for p in pair]
end
whiskercolors = Observable{RGBColors}()
lift!(plot, whiskercolors, color) do color
iszero(whiskerwidth[]) || lift!(plot, whiskercolors, color) do color
# we have twice as many linesegments for whiskers as we have errorbars, so we
# need to duplicate colors if a vector of colors is given
if color isa AbstractVector
Expand All @@ -232,7 +232,7 @@ function _plot_bars!(plot, linesegpairs, is_in_y_direction)
end
end
whiskerlinewidths = Observable{Union{Float32, Vector{Float32}}}()
lift!(plot, whiskerlinewidths, linewidth) do linewidth
iszero(whiskerwidth[]) || lift!(plot, whiskerlinewidths, linewidth) do linewidth
# same for linewidth
if linewidth isa AbstractVector
return repeat(convert(Vector{Float32}, linewidth), inner = 2)::Vector{Float32}
Expand All @@ -246,7 +246,7 @@ function _plot_bars!(plot, linesegpairs, is_in_y_direction)
colormap = colormap, colorscale = colorscale, colorrange = colorrange, inspectable = inspectable,
transparency = transparency
)
linesegments!(
iszero(whiskerwidth[]) || linesegments!(
plot, whiskers, color = whiskercolors, linewidth = whiskerlinewidths, linecap = linecap,
visible = visible, colormap = colormap, colorscale = colorscale, colorrange = colorrange,
inspectable = inspectable, transparency = transparency, space = :pixel,
Expand Down
Loading