Hint at what orientation means in the error message

[asinghvi17]

Small PR that changes the error text when you pass in e.g. boundary nodes with the wrong orientation. Feel free to close/edit if this doesn't make sense.

Should DelaunayTriangulation just reverse the order of points and go on with life?

[DanielVandH]

The advice depends on whether the curve is contiguous or has multiple sections.

julia> v = [[1, 2, 3], [3, 4, 5], [5, 6, 1]];

julia> rev = [[1, 6, 5], [5, 4, 3], [3, 2, 1]];

julia> reverse(v) # the sections don't line up
3-element Vector{Vector{Int64}}:
 [5, 6, 1]
 [3, 4, 5]
 [1, 2, 3]

julia> reverse(reverse.(v)) == rev # need to reverse sections and the entire curve
true

julia> v_contiguous = [1, 2, 3, 4, 5, 1]; rev_contiguous = [1, 5, 4, 3, 2, 1];

julia> reverse(v_contiguous) == rev_contiguous # good
true

julia> reverse(reverse.(v_contiguous))
ERROR: MethodError: no method matching reverse(::Int64)
The function `reverse` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  reverse(::Base.AnnotatedString)
   @ Base strings\annotated.jl:318
  reverse(::SubString{<:Base.AnnotatedString})
   @ Base strings\annotated.jl:328
  reverse(::Base.Order.Ordering)
   @ Base ordering.jl:56
  ...

Stacktrace:
 [1] _broadcast_getindex_evalf
   @ .\broadcast.jl:673 [inlined]
 [2] _broadcast_getindex
   @ .\broadcast.jl:646 [inlined]
 [3] getindex
   @ .\broadcast.jl:605 [inlined]
 [4] copy
   @ .\broadcast.jl:906 [inlined]
 [5] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{…}, Nothing, typeof(reverse), Tuple{…}})
   @ Base.Broadcast .\broadcast.jl:867
 [6] top-level scope
   @ REPL[31]:1
Some type information was truncated. Use `show(err)` to see complete types.

So the advice needs to be

• If the curve is contiguous, reverse(curve).
• If the curve is made of multiple sections, reverse(reverse.(curve)).

[DanielVandH]

We could pass whether the curve has multiple sections or not into the struct defining the error (see has_multiple_sections), or just breaking the advice into those two parts is probably fine..

Should DelaunayTriangulation just reverse the order of points and go on with life?

This is an option as well but I usually prefer not to modify the user's input at all and error instead. We could continue, but then there's a mismatch between their provided boundary nodes and those stored inside the Triangulation. I would also need to then add support for reversing the boundary nodes for general boundaries since they don't have to be vectors. It's easier to just error I think

If you do think that it would still be better to just flip the boundary for the user and then continue, maybe another approach is to define e.g.

function reverse_orientation(boundary_nodes)
if has_multiple_sections(boundary_nodes)
return reverse(reverse.(boundary_nodes))
elseif !has_multiple_curves(boundary_nodes)
return reverse(boundary_nodes)
end
throw(ArgumentError("Cannot reverse multiple curves at once."))
end

If reverse isn't defined for their definition of boundary_nodes this will just error anyway. You could then do a @warn (with maxlog=1 maybe) saying that the user's curve was incorrectly oriented and was flipped. If the user does have multiple curves though this is a bit annoying since we'd have to either mutate their original boundary nodes vector to put the reversed version in (so we need a set_curve! function or something) or copy it and then mutate.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.57%. Comparing base (49a6580) to head (5eec7d0).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
- Coverage   94.59%   94.57%   -0.02%     
==========================================
  Files          94       94              
  Lines        9785     9792       +7     
==========================================
+ Hits         9256     9261       +5     
- Misses        529      531       +2     

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

[DanielVandH]

What do you think of this? Wanted to do the suggestion all as one big change but it wouldn't let me...

[asinghvi17]

I didn't think of the impact on node order, it does make sense to simply error out then. Will implement a has_multiple_sections parameter in the error then!

[DanielVandH]

Will implement a has_multiple_sections parameter in the error then!

This also turned out to be annoying since the error isn't actually look at the boundary nodes it's looking at the PolygonTree. We could change it to also take the boundary nodes but that's a bit of work

[asinghvi17]

Suggested change

	str = " You can fix by passing the curve as `reverse(curve)` if the nodes are contiguous, or `reverse(reverse.(curve))` if it has multiple sections."
	str = " You can fix by passing the curve as `reverse(curve)` if the nodes are contiguous (i.e., a vector of indices), or `reverse(reverse.(curve))` if it has multiple sections (i.e., a vector of vectors of indices)."

[asinghvi17]

If this makes sense to you then feel free to merge!

[DanielVandH]

Ahh actually this advice also doesn't really work for curve-bounded domains where reverse wouldn't really be defined... I will make some edits.

[DanielVandH]

See what you think now @asinghvi17. Also added some tests.

[asinghvi17]

Looks good to me - thanks!

[DanielVandH]

Thanks!