rangebars: don't show whiskers when not needed #5004
Conversation
| @@ -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, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
I noticed these whiskers taking a significant chunk of my
profview, even when not needed (width is set to zero, which is the default). This PR makes it so that whiskers aren't computed nor drawn when not needed.What I'm not certain about, and looking for your advice, is the specific implementation. Now it checks the initial value of the whisker width only.