Use DelaunayTriangulation.jl for tricontourf

[DanielVandH]

Description

As discussed with @jkrumbiegel on Slack, this changes the triangulation package from MiniQHull to my DelaunayTriangulation.jl, enabling support for constrained triangulations. There should be no difference in the existing visualisations. As an example, here's a plot of a constrained triangulation that wouldn't be possible previously.

using CairoMakie, DelaunayTriangulation

## Start by defining the boundaries, and then convert to the appropriate interface 
curve_1 = [
    [(0.0, 0.0), (5.0, 0.0), (10.0, 0.0), (15.0, 0.0), (20.0, 0.0), (25.0, 0.0)],
    [(25.0, 0.0), (25.0, 5.0), (25.0, 10.0), (25.0, 15.0), (25.0, 20.0), (25.0, 25.0)],
    [(25.0, 25.0), (20.0, 25.0), (15.0, 25.0), (10.0, 25.0), (5.0, 25.0), (0.0, 25.0)],
    [(0.0, 25.0), (0.0, 20.0), (0.0, 15.0), (0.0, 10.0), (0.0, 5.0), (0.0, 0.0)]
] # outer-most boundary: counter-clockwise  
curve_2 = [
    [(4.0, 6.0), (4.0, 14.0), (4.0, 20.0), (18.0, 20.0), (20.0, 20.0)],
    [(20.0, 20.0), (20.0, 16.0), (20.0, 12.0), (20.0, 8.0), (20.0, 4.0)],
    [(20.0, 4.0), (16.0, 4.0), (12.0, 4.0), (8.0, 4.0), (4.0, 4.0), (4.0, 6.0)]
] # inner boundary: clockwise 
curve_3 = [
    [(12.906, 10.912), (16.0, 12.0), (16.16, 14.46), (16.29, 17.06),
    (13.13, 16.86), (8.92, 16.4), (8.8, 10.9), (12.906, 10.912)]
] # this is inside curve_2, so it's counter-clockwise 
curves = [curve_1, curve_2, curve_3]
points = [
    (3.0, 23.0), (9.0, 24.0), (9.2, 22.0), (14.8, 22.8), (16.0, 22.0),
    (23.0, 23.0), (22.6, 19.0), (23.8, 17.8), (22.0, 14.0), (22.0, 11.0),
    (24.0, 6.0), (23.0, 2.0), (19.0, 1.0), (16.0, 3.0), (10.0, 1.0), (11.0, 3.0),
    (6.0, 2.0), (6.2, 3.0), (2.0, 3.0), (2.6, 6.2), (2.0, 8.0), (2.0, 11.0),
    (5.0, 12.0), (2.0, 17.0), (3.0, 19.0), (6.0, 18.0), (6.5, 14.5),
    (13.0, 19.0), (13.0, 12.0), (16.0, 8.0), (9.8, 8.0), (7.5, 6.0),
    (12.0, 13.0), (19.0, 15.0)
]
boundary_nodes, points = convert_boundary_points_to_indices(curves; existing_points=points)
edges = Set(((1, 19), (19, 12), (46, 4), (45, 12)))

## Extract the x, y 
x = getx.(points)
y = gety.(points)
z = (x .- 1) .* (y .+ 1) 
f, ax, _ = tricontourf(x, y, z, boundary_nodes = boundary_nodes, edges = edges, levels = 30)
f

Just as a comparison with results on Master to demonstrate that there is no difference, here are two plots. The first is the new version (with the third plot now being constructed directly with a constrained triangulation rather than manually defining the triangles), and the second is on master.

There is a slight difference in the third plot as there are cocircular points, meaning the triangulation is not unique - but they are both technically correct.

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.

I'm not sure about how to go about adding the reference images, so any suggestions there would be helpful. I also wanted to add a test for compute_triangulation, but it wasn't being tested anywhere previously so I wasn't sure a good place for it.

[DanielVandH]

Not sure I understand the @constprop failures. Either way, I forgot I even had that in the code - it wasn't needed, so I removed it in DelaunayTriangulation v0.6.1. I think setting the compat to 0.6.1 is the same as [0.6.1, 0.7.0)?

[DanielVandH]

Another thought: Is there anyway to somehow allow a user to do something like

tricontouf(tri, z)

where tri is a Triangulation? I originally wanted something like e.g.

function Makie.convert_arguments(::Type{<:Tricontourf}, tri::T, z::AbstractVector{<:Real}) where {T <: DelTri.Triangulation}
    triangles = DelTri.get_triangles(tri)
    n = DelTri.num_points(tri)
    x = zeros(Float32, n)
    y = zeros(Float32, n)
    for i in DelTri.each_point_index(tri)
        p = DelTri.get_point(tri, i)
        x[i] = DelTri.getx(p)
        y[i] = DelTri.gety(p)
    end
    return (x, y, z), triangles # < - Pass triangle to attributes?
end

but I'm not sure there's a way to pass the triangles into the triangulation attribute. Maybe a preferable way is to set it up so that a user can provide the triangles as Set{NTuple{3<:Integer}} which gets converted somehow, as this is the default representation for triangles in DelaunayTriangulation.jl, although a user will still have to extract the x/y coordinates manually.

[DanielVandH]

Should work now with my changes to DelaunayTriangulation.jl. Works locally for me at least.

[asinghvi17]

Another thought: Is there anyway to somehow allow a user to do something like

tricontouf(tri, z)

where tri is a Triangulation? I originally wanted something like e.g.

function Makie.convert_arguments(::Type{<:Tricontourf}, tri::T, z::AbstractVector{<:Real}) where {T <: DelTri.Triangulation}
    triangles = DelTri.get_triangles(tri)
    n = DelTri.num_points(tri)
    x = zeros(Float32, n)
    y = zeros(Float32, n)
    for i in DelTri.each_point_index(tri)
        p = DelTri.get_point(tri, i)
        x[i] = DelTri.getx(p)
        y[i] = DelTri.gety(p)
    end
    return (x, y, z), triangles # < - Pass triangle to attributes?
end

You can return the following from convert:

PlotSpec{Tricontourf}(x, y, z; triangles = triangles)

[jkrumbiegel]

You can return the following from convert:

PlotSpec{Tricontourf}(x, y, z; triangles = triangles)

We can also make it the other way around such that the triangulation argument is the default form and the others dispatch to it.

[DanielVandH]

@jkrumbiegel: You mean having the main function be

tricontourf(tri::DelTri.Triangulation, zs; kwargs...)

? Would that be breaking with the existing triangulation keyword argument being redundant with this?

[jkrumbiegel]

No because all other methods of tricontourf would redirect to this one. If this bundles all the information needed it seems sensible to make it central.

[DanielVandH]

Of course - makes sense. I've made that change now, all seems to work on my machine. I don't know how to test the doc deployment locally though, but running the examples seems fine.

[jkrumbiegel]

I've approved the workflows, you will see whether the docs worked here. Although the preview feature won't work for a non-member-PR. If you wanted to, you could download the artifact containing the website build and look at that locally using LiveServer.jl or python

[DanielVandH]

Thanks @jkrumbiegel. How do I add the reference tests for this? It's not clear to me how to actually upload an image for it.

[jkrumbiegel]

You cannot, only admins can add the images. You can add the @reference_test code that generates the image

[DanielVandH]

Makes sense - no wonder I couldn't find the instructions for uploading :). I think these should be good.

[DanielVandH]

Did I mess something up with the reference tests for GLMakie? Or are new images just processed and uploaded differently with GLMakie than with the other backends?

[jkrumbiegel]

No, for some reason part of the ci routine that posts a message that there are missing refimages doesn't work anymore for external people, maybe that was changed for security reasons, not sure.

[ffreyer]

Do we still need the MiniQHull dependency after this pr? As far as I can tell it was only used in compute_triangulation so we should be able to remove it entirely, right?

[jkrumbiegel]

Yes, we should be able to remove it

[DanielVandH]

Indeed, didn't seem to be used anywhere else. Removed.

[DanielVandH]

Is there anything else to be done here?

[jkrumbiegel]

I don't think so, we were just pretty swamped. It's still on my to-do list :)

[jkrumbiegel]

I think this is good to go. I'll let CI run through once more, then update reference images, and run again.

[jkrumbiegel]

Thank you @DanielVandH!

[DanielVandH]

Great, thanks @jkrumbiegel!