Closed asinghvi17 closed 1 month ago
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
reverse(curve)
.reverse(reverse.(curve))
.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.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 94.57%. Comparing base (
49a6580
) to head (5eec7d0
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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!
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
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.
See what you think now @asinghvi17. Also added some tests.
Looks good to me - thanks!
Thanks!
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?