Fix bug of polygon calculation for voronoiplots (#234)

[maltepuetz]

This should fix: #234

I added the internal function _grow_polygon_outside_of_box, which is essentially grow_polygon_outside_of_box, but takes a type (e.g. ::Float64) as additional input.

The original function grow_polygon_outside_of_box is now a wrapper for the new internal function. It runs the internal function with ::Float64 (this should have the same behavior as before) and if this fails (i.e. if it returns new_pointswith values that are Inf, it runs the internal function again with t, p and q being of type ::BigFloat.

This ensures higher precision and fixed the issue. Creating the wrapper and running the Float64 version first ensures that performance does not decrease from this fix.

I almost entirely copied the docstring of the original function for the internal function. Not sure if you want to keep it like that? Also let me know if you have a better idea of fixing this! :)

[DanielVandH]

If you don't mind, can you also check if this fixes #223?

[DanielVandH]

Also let me know if you have a better idea of fixing this! :)

I had some ideas to try and improve this function but, rather than letting this fix get in the way of me waiting till I might eventually have time to even begin looking into it, this is a great help! I appreciate it.

[maltepuetz]

If you don't mind, can you also check if this fixes #223?

It does not seem to fix #223, I still get the same figure with one of the cells acting weird.

[DanielVandH]

Alright, I figured as much but might've been a nice side-effect!

Can you also add a test for your #234 please?

[maltepuetz]

Can you also add a test for your #234 please?

Will do the test tomorrow, not sure where to put it. Should I put it as a new testset inside https://github.com/JuliaGeometry/DelaunayTriangulation.jl/blob/main/test/voronoi/voronoi.jl?

[DanielVandH]

Should I put it as a new testset inside https://github.com/JuliaGeometry/DelaunayTriangulation.jl/blob/main/test/voronoi/voronoi.jl?

Yeah, there's good. And for allfinite maybe some tests at the end of

https://github.com/JuliaGeometry/DelaunayTriangulation.jl/blob/main/test/interfaces/points.jl

would be good. I think these tests are sufficient

@testset "allfinite" begin
    # All finite points
    @test allfinite([[2.0, 3.5], [1.7, 23.3], [-1.0, 0.0]])
    @test allfinite([(2.0, 3.5), (1.7, 23.3), (-1.0, 0.0)])
    @test allfinite([2.0 1.7 -1.0; 3.5 23.3 0.0])
    @test allfinite(((2.0, 3.5), (1.7, 23.3), (-1.0, 0.0)))
    
    # With Inf
    @test !allfinite([[2.0, 3.5], [Inf, 23.3], [-1.0, 0.0]])
    @test !allfinite([(2.0, 3.5), (Inf, 23.3), (-1.0, 0.0)])
    @test !allfinite([2.0 Inf -1.0; 3.5 23.3 0.0])
    @test !allfinite(((2.0, 3.5), (Inf, 23.3), (-1.0, 0.0)))
    
    # With NaN
    @test !allfinite([[2.0, 3.5], [1.7, NaN], [-1.0, 0.0]])
    @test !allfinite([(2.0, 3.5), (1.7, NaN), (-1.0, 0.0)])
    @test !allfinite([2.0 1.7 NaN; 3.5 23.3 0.0])
    @test !allfinite(((2.0, 3.5), (1.7, NaN), (-1.0, 0.0)))
    
    # With both Inf and NaN
    @test !allfinite([[2.0, 3.5], [Inf, 23.3], [NaN, 0.0]])
    
    # Edge cases
    @test allfinite(Vector{Vector{Float64}}())  # vacuous truth
    @test allfinite(Matrix{Float64}(undef, 2, 0))  # vacuous truth
    @test allfinite([[1.0, 2.0]])
    @test !allfinite([[Inf, 2.0]])
    
    # First point non-finite
    @test !allfinite([[Inf, 1.0], [2.0, 3.0]])
    @test !allfinite([[NaN, 1.0], [2.0, 3.0]])
    
    # Last point non-finite
    @test !allfinite([[1.0, 2.0], [3.0, 4.0], [5.0, Inf]])
    @test !allfinite([[1.0, 2.0], [3.0, 4.0], [NaN, 6.0]])
end

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.87%. Comparing base (c0305a9) to head (268ac08).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
+ Coverage   94.81%   94.87%   +0.06%     
==========================================
  Files         102      102              
  Lines       10356    10370      +14     
==========================================
+ Hits         9819     9839      +20     
+ Misses        537      531       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[DanielVandH]

Can you test with validate_tessellation instead? You can see how it's used in the rest of that voronoi test file. There's not really much to gain from a @test true. Or e.g., similar to https://github.com/maltepuetz/DelaunayTriangulation.jl/blob/8f8cc03432abb0aa6acde41e11a86da8df7d17db/test/voronoi/voronoi.jl#L1021-L1043, you can get the polygon coordinates for the tiles that were previously broken and test that they now match what's correct

[maltepuetz]

Can you test with validate_tessellation instead? You can see how it's used in the rest of that voronoi test file. There's not really much to gain from a @test true.

I don't think

for n in 2:10
    points = generatePoints(n)
    points2d = [projection(p) for p in points]
    tri = triangulate(points2d)
    vorn = voronoi(tri)
    @test validate_tessellation(vorn)
end

is the way to go here, as this is failing for n > 4 (regardless of whether I run the code pre- or postfix -- also this is not calling grow_polygon_outside_of_box as far as I can tell). Maybe there is another, related issue? I am not familiar with the logic behind the algorithms, so I cannot tell. My original tests

for n in 2:10
    points = generatePoints(n)
    points2d = [projection(p) for p in points]
    @test voronoiplot([p[1] for p in points2d], [p[2] for p in points2d], ones(size(points2d))) |> _ -> true
end

fail for n > 3 prefix, but pass postfix.

Or e.g., similar to https://github.com/maltepuetz/DelaunayTriangulation.jl/blob/8f8cc03432abb0aa6acde41e11a86da8df7d17db/test/voronoi/voronoi.jl#L1021-L1043, you can get the polygon coordinates for the tiles that were previously broken and test that they now match what's correct

I changed the tests, such that they compare the correct polygon coordinates with the polygon coordinates from DT.get_polygon_coordinates. The error in the code prefix is actually not happening in the comparison, but upon calling DT.get_polygon_coordinates, which fails to calculate the coordinates. What do you think?

[DanielVandH]

Your voronoi call isn't the same as the one that voronoiplot uses (it has some clipping it used when it gets the polygon coordinates, that's the difference you're seeing). I'll try and take a look at it myself soon since that might be a bit simpler.

[DanielVandH]

I'm also happy if you just want to do a reference test. I do that here

https://github.com/maltepuetz/DelaunayTriangulation.jl/blob/8f8cc03432abb0aa6acde41e11a86da8df7d17db/test/refinement/refine.jl#L2805

for example. You just need to add using ReferenceTests to the top of the file. Also in your tests for allfinite, in the same file you need, at the top, using DelaunayTriangulation: allfinite. I think after these two things, and if you bump the version in Project.toml to 1.6.6 we should be good to go? Just also show me the reference test images (what you get prior to this patch and what you get on this patch).

[maltepuetz]

I'm also happy if you just want to do a reference test.

Yes, will do. Should I replace the current tests or just keep them?

I think after these two things, and if you bump the version in Project.toml to 1.6.6 we should be good to go?

I think so too 👍

Just also show me the reference test images (what you get prior to this patch and what you get on this patch).

Prior to this patch you just get an error message, but I'll show you the images post patch.

[DanielVandH]

Yes, will do. Should I replace the current tests or just keep them?

Just replace the tests with an appropriate reference test, it's fine. Keep the allfinite tests obviously (with using DelaunayTriangulation: allfinite at the top of that test file).

[maltepuetz]

These are the two reference figures:

I think everything should work now.

[DanielVandH]

Thanks!