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

Deprecate resolution for size #3343

Merged
merged 7 commits into from
Nov 1, 2023
Merged

Conversation

jkrumbiegel
Copy link
Member

Description

In 0.20, resolution stops being a pixel resolution in GLMakie. It was already not really true for CairoMakie before due to px_per_unit scaling. The term size is more neutral and also works better with the function resize! which we already used to change this value. A warning will now be emitted when a Scene is created while resolution is in the theme. In that case, the resolution value will be preferred over size to avoid breaking old code completely.

@MakieBot
Copy link
Collaborator

MakieBot commented Nov 1, 2023

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(display(fig))
using create display create display
GLMakie 5.00s (4.97, 5.08) 0.04+- 398.29ms (390.62, 408.96) 5.99+- 776.91ms (759.05, 813.99) 18.76+- 10.49ms (10.38, 10.57) 0.06+- 117.83ms (116.56, 118.88) 0.92+-
sd/beta-20 4.99s (4.97, 5.01) 0.01+- 396.40ms (391.87, 402.35) 3.62+- 736.18ms (727.96, 740.74) 4.41+- 10.34ms (10.21, 10.51) 0.09+- 75.55ms (74.32, 76.88) 0.82+-
evaluation +0.20%, 0.01s invariant (0.37d, 0.51p, 0.02std) +0.48%, 1.9ms invariant (0.38d, 0.49p, 4.80std) +5.24%, 40.73ms slower❌ (2.99d, 0.00p, 11.59std) +1.43%, 0.15ms slower X (1.92d, 0.00p, 0.08std) +35.88%, 42.28ms slower❌ (48.31d, 0.00p, 0.87std)
CairoMakie 4.25s (4.23, 4.27) 0.01+- 403.89ms (398.68, 413.27) 5.13+- 375.23ms (372.99, 377.76) 1.64+- 10.76ms (10.28, 10.90) 0.22+- 5.54ms (5.08, 5.87) 0.25+-
sd/beta-20 4.23s (4.20, 4.25) 0.02+- 397.41ms (393.90, 401.64) 3.37+- 369.82ms (366.64, 373.04) 2.43+- 10.80ms (10.68, 10.87) 0.07+- 4.94ms (4.85, 5.05) 0.09+-
evaluation +0.45%, 0.02s slower X (1.23d, 0.04p, 0.02std) +1.60%, 6.48ms slower X (1.49d, 0.02p, 4.25std) +1.44%, 5.4ms slower X (2.60d, 0.00p, 2.04std) -0.33%, -0.04ms invariant (-0.22d, 0.69p, 0.14std) +10.80%, 0.6ms slower❌ (3.21d, 0.00p, 0.17std)
WGLMakie 3.33s (3.31, 3.36) 0.02+- 501.20ms (491.17, 526.88) 12.99+- 7.54s (7.41, 7.62) 0.08+- 8.73ms (8.32, 10.83) 0.92+- 469.56ms (465.87, 473.21) 2.74+-
sd/beta-20 3.33s (3.30, 3.36) 0.03+- 493.90ms (488.86, 501.27) 4.55+- 7.49s (7.37, 7.55) 0.06+- 7.62ms (7.55, 7.78) 0.08+- 406.83ms (400.21, 415.98) 5.99+-
evaluation +0.05%, 0.0s invariant (0.06d, 0.91p, 0.02std) +1.46%, 7.31ms invariant (0.75d, 0.20p, 8.77std) +0.64%, 0.05s invariant (0.67d, 0.24p, 0.07std) +12.76%, 1.11ms slower❌ (1.70d, 0.02p, 0.50std) +13.36%, 62.73ms slower❌ (13.46d, 0.00p, 4.37std)

@SimonDanisch
Copy link
Member

It's weird that the regressions in the display benchmark seem somewhat consistent, but locally I can't reproduce them, and from the code changes I'd be quite surprised as well...
It even seems that most of the time this PR is faster 🤷
(sd/beta-20 vs this PR)
image

@jkrumbiegel jkrumbiegel merged commit 6a6d1c4 into sd/beta-20 Nov 1, 2023
13 checks passed
@jkrumbiegel jkrumbiegel deleted the jk/rename-resolution-to-size branch November 1, 2023 10:41
@jkrumbiegel
Copy link
Member Author

yeah I don't think that makes so much sense, if so we can follow up later

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.

3 participants