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

Rafaqz allow zero length #2917

Merged
merged 6 commits into from May 5, 2023
Merged

Rafaqz allow zero length #2917

merged 6 commits into from May 5, 2023

Conversation

SimonDanisch
Copy link
Member

Clean up of #2911 ...
@jkrumbiegel, did we have any reason to close polygons, if they were already closed?
I now added a check here:
image

So we don't already close closed polygons... Weirdly enough, it looks like we specifically tested for this

@SimonDanisch SimonDanisch marked this pull request as ready for review May 5, 2023 12:43
@MakieBot
Copy link
Collaborator

MakieBot commented May 5, 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 40.68s (40.01, 41.47) 0.48+- 18.41s (18.10, 18.79) 0.26+- 17.08s (16.47, 18.15) 0.53+- 12.09ms (11.88, 12.34) 0.16+- 142.12ms (136.12, 148.14) 3.78+-
master 40.93s (40.46, 41.50) 0.35+- 18.61s (18.03, 18.89) 0.29+- 17.05s (16.76, 17.62) 0.32+- 12.19ms (11.97, 12.65) 0.25+- 141.35ms (134.98, 145.27) 3.67+-
evaluation -0.62%, -0.25s invariant (-0.60d, 0.28p, 0.41std) -1.08%, -0.2s invariant (-0.71d, 0.21p, 0.28std) +0.13%, 0.02s invariant (0.05d, 0.93p, 0.42std) -0.84%, -0.1ms invariant (-0.48d, 0.39p, 0.21std) +0.54%, 0.77ms invariant (0.21d, 0.70p, 3.73std)
CairoMakie 34.63s (34.40, 34.79) 0.14+- 17.36s (17.23, 17.47) 0.10+- 2.53s (2.51, 2.61) 0.04+- 10.87ms (10.80, 10.91) 0.04+- 5.26ms (5.14, 5.42) 0.10+-
master 34.68s (34.57, 34.80) 0.08+- 17.25s (17.06, 17.47) 0.15+- 2.52s (2.47, 2.56) 0.03+- 10.79ms (10.72, 10.86) 0.05+- 5.18ms (5.07, 5.58) 0.19+-
evaluation -0.15%, -0.05s invariant (-0.44d, 0.43p, 0.11std) +0.66%, 0.11s invariant (0.90d, 0.12p, 0.12std) +0.60%, 0.02s invariant (0.45d, 0.41p, 0.03std) +0.68%, 0.07ms slower X (1.59d, 0.01p, 0.05std) +1.63%, 0.09ms invariant (0.58d, 0.31p, 0.14std)
WGLMakie 47.58s (47.08, 47.82) 0.26+- 23.69s (23.60, 23.85) 0.10+- 24.29s (24.08, 24.71) 0.23+- 17.19ms (16.07, 18.77) 1.07+- 864.16ms (828.83, 906.70) 28.77+-
master 47.51s (47.23, 47.74) 0.17+- 23.61s (23.43, 23.84) 0.18+- 24.35s (24.14, 24.70) 0.20+- 17.66ms (16.80, 18.04) 0.46+- 853.70ms (805.57, 882.94) 23.71+-
evaluation +0.15%, 0.07s invariant (0.32d, 0.57p, 0.22std) +0.36%, 0.09s invariant (0.58d, 0.30p, 0.14std) -0.27%, -0.07s invariant (-0.30d, 0.58p, 0.22std) -2.71%, -0.47ms invariant (-0.57d, 0.32p, 0.76std) +1.21%, 10.46ms invariant (0.40d, 0.47p, 26.24std)

@SimonDanisch SimonDanisch requested a review from rafaqz May 5, 2023 13:29
Copy link
Contributor

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

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

LGTM

(I did wonder why you were always closing the polygons)

@SimonDanisch SimonDanisch merged commit fa79f3d into master May 5, 2023
14 checks passed
@SimonDanisch SimonDanisch deleted the rafaqz-allow_zero_length branch May 5, 2023 14:17
@SimonDanisch SimonDanisch mentioned this pull request May 5, 2023
7 tasks
@jkrumbiegel
Copy link
Collaborator

Not sure, could have been something Cairo related at the time. Let's just watch out for visual regressions.

@SimonDanisch
Copy link
Member Author

I did check the reference image tests explicitly, to make sure there are no changes below threshold.

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.

None yet

4 participants