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

Fix type inference failure in get_clims #3838

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

BioTurboNick
Copy link
Member

Resolves #3837

The intent is to short-circuit the clims calculation with a shape series type. Each shape is a series and only has a single color (right?), so all that calculation is unnecessary.

Only issue with the current PR is that using fill_z instead of fillcolor is broken, plot shapes have no color. Has to be a way to detect this situation to allow get_clims to work properly?

@BeastyBlacksmith
Copy link
Member

Each shape is a series and only has a single color (right?)

I don't think so, you should be able to set the color of the border separate from the fillcolor and while not implemented we shouldn't block the way to fill shapes with gradients

@BioTurboNick
Copy link
Member Author

Fair enough.

Taking a closer look, it seems that code is very type unstable, or inference failed. If I enforce the type of the return value as Tuple{Float64, Float64}, performance improves almost as much.

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #3838 (37a6476) into master (0cd8124) will decrease coverage by 0.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3838      +/-   ##
==========================================
- Coverage   63.50%   63.25%   -0.26%     
==========================================
  Files          28       28              
  Lines        7464     7489      +25     
==========================================
- Hits         4740     4737       -3     
- Misses       2724     2752      +28     
Impacted Files Coverage Δ
src/colorbars.jl 92.72% <100.00%> (+1.81%) ⬆️
src/utils.jl 52.85% <0.00%> (-1.93%) ⬇️
src/args.jl 73.46% <0.00%> (-0.58%) ⬇️
src/backends/pgfplotsx.jl 61.17% <0.00%> (-0.38%) ⬇️
src/backends.jl 63.47% <0.00%> (ø)
src/examples.jl 61.97% <0.00%> (ø)
src/backends/unicodeplots.jl 75.00% <0.00%> (ø)
src/backends/gr.jl 89.42% <0.00%> (+0.08%) ⬆️

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 0cd8124...37a6476. Read the comment docs.

@BioTurboNick BioTurboNick changed the title For shape type, cut off slow, unnecessary process Fix type inference failure in get_clims Sep 22, 2021
Copy link
Member

@t-bltg t-bltg left a comment

Choose a reason for hiding this comment

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

Nice, we should have more of these to be as type stable as possible.

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

Successfully merging this pull request may close these issues.

get_clims as bottleneck in plot rendering
3 participants