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

Delaunay recipes #3159

Merged
merged 79 commits into from
Aug 22, 2023
Merged

Delaunay recipes #3159

merged 79 commits into from
Aug 22, 2023

Conversation

SimonDanisch
Copy link
Member

Continuation of #3102

@MakieBot
Copy link
Collaborator

MakieBot commented Aug 16, 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 14.18s (13.05, 15.14) 0.69+- 1.34s (1.28, 1.45) 0.07+- 942.30ms (908.66, 971.39) 22.98+- 16.42ms (15.84, 16.95) 0.36+- 191.63ms (188.08, 196.77) 2.98+-
master 13.58s (13.07, 14.15) 0.38+- 1.32s (1.24, 1.43) 0.08+- 954.34ms (894.85, 988.36) 32.20+- 16.57ms (16.03, 17.01) 0.41+- 194.30ms (187.98, 197.15) 3.07+-
evaluation +4.22%, 0.6s invariant (1.07d, 0.07p, 0.53std) +1.53%, 0.02s invariant (0.28d, 0.61p, 0.07std) -1.28%, -12.04ms invariant (-0.43d, 0.44p, 27.59std) -0.91%, -0.15ms invariant (-0.38d, 0.49p, 0.39std) -1.39%, -2.67ms invariant (-0.88d, 0.12p, 3.03std)
CairoMakie 11.70s (11.50, 12.12) 0.20+- 1.28s (1.25, 1.31) 0.02+- 258.59ms (242.18, 271.21) 9.08+- 15.38ms (15.24, 15.58) 0.13+- 8.49ms (8.43, 8.63) 0.07+-
master 11.62s (11.47, 11.76) 0.10+- 1.24s (1.20, 1.28) 0.03+- 304.70ms (292.17, 317.87) 9.70+- 15.34ms (15.11, 15.55) 0.13+- 8.50ms (8.36, 8.63) 0.09+-
evaluation +0.69%, 0.08s invariant (0.51d, 0.37p, 0.15std) +2.44%, 0.03s slower X (1.34d, 0.03p, 0.02std) -17.83%, -46.11ms faster✅ (-4.91d, 0.00p, 9.39std) +0.21%, 0.03ms invariant (0.24d, 0.66p, 0.13std) -0.07%, -0.01ms invariant (-0.07d, 0.89p, 0.08std)
WGLMakie 16.44s (16.12, 16.88) 0.29+- 1.58s (1.53, 1.64) 0.03+- 15.02s (14.76, 15.42) 0.27+- 19.17ms (18.26, 20.74) 0.84+- 1.47s (1.36, 1.56) 0.07+-
master 16.56s (16.34, 16.77) 0.17+- 1.59s (1.48, 1.69) 0.07+- 15.05s (14.91, 15.25) 0.14+- 18.87ms (17.26, 19.75) 0.89+- 1.48s (1.44, 1.52) 0.03+-
evaluation -0.75%, -0.12s invariant (-0.52d, 0.35p, 0.23std) -0.58%, -0.01s invariant (-0.17d, 0.76p, 0.05std) -0.24%, -0.04s invariant (-0.17d, 0.76p, 0.20std) +1.57%, 0.3ms invariant (0.35d, 0.53p, 0.86std) -1.03%, -0.02s invariant (-0.27d, 0.62p, 0.05std)

@SimonDanisch
Copy link
Member Author

Can someone check:

https://docs.makie.org/previews/PR3159/examples/plotting_functions/voronoiplot/
https://docs.makie.org/previews/PR3159/examples/plotting_functions/triplot/
And best check:

# from the project where you developped Makie
using ReferenceUpdater; ReferenceUpdater.serve_update_page(pr=3102)

I'm not 100% sure what the right output is supposed to look!
If this all looks good, I think we can merge, right?

docs/examples/plotting_functions/voronoiplot.md Outdated Show resolved Hide resolved
docs/examples/plotting_functions/voronoiplot.md Outdated Show resolved Hide resolved
docs/examples/plotting_functions/voronoiplot.md Outdated Show resolved Hide resolved
docs/examples/plotting_functions/voronoiplot.md Outdated Show resolved Hide resolved
src/basic_recipes/voronoiplot.jl Outdated Show resolved Hide resolved
@DanielVandH
Copy link
Contributor

Can't edit but I think those changes I've just reviewed will make the docs look better.

SimonDanisch and others added 2 commits August 17, 2023 08:59
Co-authored-by: Daniel VandenHeuvel <95613936+DanielVandH@users.noreply.github.com>
@ffreyer
Copy link
Collaborator

ffreyer commented Aug 17, 2023

I simplified a few docs examples and updated the bounding_box docstring. Regarding bounding_box I'm wondering if

  • we should add Rect2 -> tuple conversion for triplot to make it more Makie-like
  • we should rename bounding_box to clip for voronoiplot since we have the Circle clipping in there too

Also, is this correct with the pink line cutting through? (From docs)

@DanielVandH
Copy link
Contributor

we should add Rect2 -> tuple conversion for triplot to make it more Makie-like

This would be a good idea.

we should rename bounding_box to clip for voronoiplot since we have the Circle clipping in there too

I think that's okay, go for it.

@github-actions
Copy link
Contributor

Missing reference images

Found 8 new images without existing references.
Upload new reference images before merging this PR.

@SimonDanisch
Copy link
Member Author

So... Is this ready now? ^^ I've been updating the refimages quite often, so if this isn't ready yet, I won't update them right away ;)

@DanielVandH
Copy link
Contributor

As far as I can tell, everything is good. Those failed tests worked fine for me locally - did something go wrong with them in the test, or were they just not updated properly? If it's the latter, then I'm happy with it all

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 22, 2023

I just realized that a lot of tests pass the RNG with triangulate(..., rng = RNG.STABLE_RNG) but a lot also don't. Are the ones that don't at risk of failing in the future?

@DanielVandH
Copy link
Contributor

For all those that don't, the triangulation is unique and so there is no risk of failure. For Voronoi diagrams, even if the triangulation is non-unique then the diagram is still unique. So, there should be no risk of failure (passing rng = RNG.STABLE_RNG into those is still fine if you would like to, but I just didn't need to).

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 22, 2023

Ok, I just got worried for a second. From my side this is good to go as well

@SimonDanisch SimonDanisch merged commit a4af178 into master Aug 22, 2023
13 checks passed
@SimonDanisch SimonDanisch deleted the delaunay_recipes branch August 22, 2023 11:17
@DanielVandH
Copy link
Contributor

Hurrah! Thank you @SimonDanisch and @ffreyer :)

Do you know when a new Makie version might be registered so that these functions are available again @SimonDanisch? No rush, just want to check.

@SimonDanisch
Copy link
Member Author

Trying to tag in the next day or two!

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.

4 participants