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

Add triplot and voronoiplot from DelaunayTriangulation.jl #3102

Closed
wants to merge 146 commits into from
Closed

Add triplot and voronoiplot from DelaunayTriangulation.jl #3102

wants to merge 146 commits into from

Conversation

DanielVandH
Copy link
Contributor

@DanielVandH DanielVandH commented Jul 28, 2023

Description

This PR adds triplot and voronoiplot from DelaunayTriangulation.jl (see here for the original code) to fix the circular dependency issue mentioned from @SimonDanisch here. Just for reference, here's an example of what it looks like:

fig = Figure()
ax1 = Axis(fig[1, 1],width=400,height=400)
ax2 = Axis(fig[1, 2],width=400,height=400)
pts = tuple.(rand(100), rand(100))
tri = triangulate(pts)
refine!(tri; max_area = 1e-3)
vorn = voronoi(tri, true)
triplot!(ax1, tri)
voronoiplot!(ax2, vorn)
resize_to_layout!(fig)

image

Once this is in (and after adding any other suggestions / changes to these recipes that might be made), I'll remove the corresponding code from DelaunayTriangulation.jl so that it is Makie-free.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@DanielVandH
Copy link
Contributor Author

Worth adding anything from this to the documentation? I figured not since it is pretty DelaunayTriangulation.jl specific.

Two other comments:

  1. While adding this I tried fixing some of the Observables issues, e.g. I want this to work:
tri = triangulate([rand(2) for _ in 1:50])
_tri = Observable(tri)
fig, ax, sc = triplot(_tri)
map(_tri) do tri 
	refine!(tri, max_area=1e-4get_total_area(tri))
end # Plot should now be updated 

Is my understanding of how this should work incorrect?

  1. I want to be able to truncate the ghost edges properly when they are plotted. I thought xautolimits and yautolimits should work, as I've done in triplot, but it doesn't seem to do anything:
tri = triangulate(rand(2, 50))
triplot(tri, show_ghost_edges = true)

image

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 28, 2023

I want to be able to truncate the ghost edges properly when they are plotted. I thought xautolimits and yautolimits should work, as I've done in triplot, but it doesn't seem to do anything

autolimits doesn't really truncate the edge anyway. It just tells axis whether to consider this plot for limit calculations. You can still zoom in and out after that. To actually remove these lines you'd need to remove the data points responsible for them. Maybe you can check for points outside a bounding box or check line lengths for that?

@DanielVandH
Copy link
Contributor Author

I want to be able to truncate the ghost edges properly when they are plotted. I thought xautolimits and yautolimits should work, as I've done in triplot, but it doesn't seem to do anything

autolimits doesn't really truncate the edge anyway. It just tells axis whether to consider this plot for limit calculations. You can still zoom in and out after that. To actually remove these lines you'd need to remove the data points responsible for them. Maybe you can check for points outside a bounding box or check line lengths for that?

Oh, is that how it's supposed to work? I tried autolimits!(ax) but it didn't do anything either though. I could check for a bounding box, but it's hard to determine one in case e.g. a user changes the limits afterwards. Really it would be nice to have it be handled like h/ab/vlines! does, but for a ray instead of a line.
It's not a big deal if this can't work, it's not common that someone wants to check a ghost edge anyway.

@ffreyer
Copy link
Collaborator

ffreyer commented Jul 28, 2023

Oh are they supposed to fill out the rectangle of the Axis? In that case I would suggest you create your own rectangle in the recipe and just stop them there. Then you can define needs_tight_limits(::PlotType) = true so that the axis behaves like it does for a heatmap plot

@DanielVandH
Copy link
Contributor Author

Didn't know about needs_tight_limits, that seems useful. I'll try and come up with a reasonable default for a rectangle size and then implement that, then.

@DanielVandH
Copy link
Contributor Author

Works a treat! Thanks for that suggestion.

triplot(triangulate(rand(2, 50)), show_ghost_edges = true)

image

@jkrumbiegel
Copy link
Collaborator

needs_tight_limits

@ffreyer I was actually hoping we could remove this at some point. It's kind of an axis setting set by a plotting function (setting xautolimitmargin and y... to (0, 0)` and I'm not sure if it's the wrong level of abstraction. I just had to add it at the time so that making heatmaps and images isn't as annoying..

@SimonDanisch
Copy link
Member

Maybe we should release a breaking DelaunayTriangulation.jl release without Makie and rely on that here?

@DanielVandH
Copy link
Contributor Author

DanielVandH commented Jul 31, 2023 via email

@SimonDanisch
Copy link
Member

I'm trying to tag a patch release this week!

@DanielVandH
Copy link
Contributor Author

I've released the new version now (and then realised I have a bunch of edge cases to fix with the plots :) ). The test failures seem to just be from old reference images after I updated the recipes a bit.

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 15, 2023

Did a rebase master. Hopefully everything is fine now

@DanielVandH
Copy link
Contributor Author

e: Didn't see that I somehow messed up a bunch of files when going back to this branch after creating the linked PR. Does it matter? Is it just the merged master?

Ah. Guess it wasn't the merged master. Don't know how I managed that. Sorry for mixing up the files like that - thanks for fixing it @ffreyer.

@DanielVandH
Copy link
Contributor Author

DanielVandH commented Aug 16, 2023

I'm not sure how I managed to make the same mistake again? I'm pretty sure all I did was pull within VSCode ... Will undo once I learn how (: And then I will just stop touching it

I've tried a few things but my Git seems really messed up with this branch for some reason? I've no clue what I've done to it honestly. Sorry to bug you again @ffreyer, are you able to redo the rebase master? I feel like anything I'm about to do will only make matters worse.

@SimonDanisch SimonDanisch mentioned this pull request Aug 16, 2023
@SimonDanisch
Copy link
Member

As long as you don't force push, it should just be a matter of undoing the last commit!
Although, it does seem like it isn't working with git reset --hard HEAD~1...
Had to do git reset --hard 55c2201c6.
And yes, pulling from a branch that got forced pushed to isn't straightforward...
To be honest, I always do git reset --hard HEAD~20 (some big enough number), and then pull again :D
I think the correct way is to do:

git reset --hard origin/your-branch

I opened a new PR with a corrected version since its easier to work with and actually builds + publishes the docs!
#3159

@DanielVandH DanielVandH deleted the delaunay_recipes branch August 16, 2023 22:47
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