Speed up get_clims

[BioTurboNick]

Finally resolves #3837 , I think. (That was faster than I thought!)

Plots.frame(anim)
Before: 8.361 s (11501264 allocations: 342.00 MiB)
After: 697.416 ms (1869572 allocations: 47.78 MiB)

This change stores the color limits as a property of the subplot, which it looks up instead of iterating over all series again.

Since it's switching from a dynamic lookup to a stored value though, need to pay attention to when else it needs to be explicitly updated. E.g. if fill_z or related is changed that would affect existing series. Right?

[BioTurboNick]

And with this change, the profiler shows the ~10% of the time is spent doing type inference, and the rest of the time drawing the shapes. get_clims doesn't even show up in the profiler, at least above 10 counts.

[codecov]

Codecov Report

Merging #3839 (202bed0) into master (032c5d1) will decrease coverage by 0.08%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #3839      +/-   ##
==========================================
- Coverage   63.23%   63.15%   -0.09%     
==========================================
  Files          28       28              
  Lines        7491     7491              
==========================================
- Hits         4737     4731       -6     
- Misses       2754     2760       +6     

Impacted Files	Coverage Δ
src/backends/pgfplotsx.jl	61.17% <0.00%> (ø)
src/utils.jl	52.85% <0.00%> (ø)
src/colorbars.jl	90.74% <90.00%> (-1.99%)	⬇️
src/backends/unicodeplots.jl	75.39% <100.00%> (ø)
src/pipeline.jl	90.63% <100.00%> (+0.04%)	⬆️
src/axes.jl	84.73% <0.00%> (-0.44%)	⬇️
src/recipes.jl	59.27% <0.00%> (-0.22%)	⬇️
src/backends/gr.jl	89.25% <0.00%> (-0.09%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c89bd8...202bed0. Read the comment docs.

[isentropic]

Looks good to me

[isentropic]

Do I understand it right that when you animate the plots, the clims are no longer calculated again?

[isentropic]

Is there a chance that when animating, the clims would not reflect the right color if the animation would change the colorrange

[BioTurboNick]

This is my understanding:

1. clims are calculated from get_<x>color-family methods when no clims are passed in, but it only calculates it within all series a single subplot. This was not touched, and is likely fine as it seems to only be called a fixed number of times per subplot, e.g. when rendering a subplot's colorbar.

2. clims are calculated when various backends add a series to the rendering; and if colorbar_entry is true, which is the default, it iterates over all series in the subplot. This was the key problem.

3. Animations are constructed by layering complete, rendered plot frames on top of each other. There're no cross-plot calculations happening inside the animation methods.

4. However, if it's possible to change values that would affect clims after they have been set (e.g. fill_z), then it's possible that the plot would not reflect those changes. Those changes need to be accounted for.

[BeastyBlacksmith]

Here are the results for

julia> using Plots
julia> plot(1:5, line_z = 1:5 )
julia> plot!(2:6, line_z = range(-10,10, length=5))

this PR (png)

this PR (svg)

this PR (vscode plot pane)

master

[BeastyBlacksmith]

so something is different between display and savefig here

[BioTurboNick]

Thanks for the example, I'll check it out.

[BioTurboNick]

I'm having trouble figuring out where the right place would be to update a stored clim value.

args.jl has _update_subplot_args which calls an empty "dynamic callback" _update_subplot_colorbars, but this all runs before the series has been added to the subplot, so series information can't be used to set color limits here.

There's also _update_series_attributes!, but it also runs before any series have been added to the subplot. (Also there's no Series object being used here?)

[BioTurboNick]

Just to get it to work, I stuck it at the end of the pipeline.

So now it:

1. Updates :colorbar_limits whenever a series is added
2. When a function requests a subplot's clims, it just reads from that value.
3. When a function requests a series' clims, if the series is part of the colorbar (default), it looks up the precalculated value for the subplot. If the series is not part of the colorbar, it calculates the series' color limits freshly.

[BioTurboNick]

I just noticed there's already a sp[:clims] that's doing something here in the default argument...

EDIT: Ah, that's so the user can define their own, but if color values show up beyond that range, it'll be expanded. Any reason to not just overwrite :clims then, instead of a separate entry?

[BioTurboNick]

Alright, I understand this all better now.

:clims subplot setting is being relied on to know if the user has explicitly set them, and the automatic process will be overridden. So that has to remain untouched. I'm using :crange instead for that purpose. This is what the op argument allowed.

These changes mean I had to add an update call in the aforementioned dynamic callback to handle a following call like plot!(clims = (-10, 20))

[t-bltg]

@BioTurboNick thanks for the investigation on that matter. Is the PR ready ?

[BioTurboNick]

Ready for review, yes.

[t-bltg]

#3839 (comment) looks ok: png, svg and gui.

I trust you on the performance improvement 😉.

[BioTurboNick]

Thanks! The remaining issue is just where the update calls are coming from. I just changed it to call _update_subplot_colorbars, which then calls update_clims... That may be a good-enough structure?

[BeastyBlacksmith]

That works well as far as I can see. If you resolve conflicts, I'll merge this.
Thanks a lot!