JuliaGeometry / VoronoiDelaunay.jl

Fast and robust Voronoi & Delaunay tessellation creation with Julia
Other
124 stars 26 forks source link

Can we automatically handle points outside of the [1,2] interval? #27

Closed juliohm closed 1 year ago

juliohm commented 7 years ago

I understood that we have to manually normalize point coordinates to the square [1,2]^2. Why not do this step for the user automatically? IMHO, this would improve the usability of the package by a lot.

skariel commented 7 years ago

doing so automatically is not the best for performance, which is a big goal of this package. For e.g. if the user can use appropriate coordinates without rescaling, then this automatic rescaling would just waste time

juliohm commented 7 years ago

So make it an option that is disabled by default. I value performance too, but we shouldn't have an ugly API for things, specially when we are talking Julia: performance + clean code.

If the user wants to manually scale things, fine, but in most use cases people don't want to deal with it. Plus, if a new user doesn't read the README carefully he/she will run the code without scaling and get unexpected behavior. What is the outcome? Users complaining about bugs that don't exist.

skariel commented 7 years ago

I agree overall, but it's not that simple. For example if inserts immutable points and then iterates over edges, the points will have to be rescaled in the iteration, regardless of the inner representation. One simple thing we could do for safety is for the default functions to assert the points inserted are in the correct region

juliohm commented 7 years ago

Yes, this is exactly the idea. Re-scale the points inside of the tessellation algorithm and map them back to the user. If someone doesn't want to have the re-scaling happening inside of the tessellation algorithm for some reason that I still don't understand, they will pass an option to disable it (e.g. scale=false) and will handle the scaling in the way they wish.

gabor1 commented 6 years ago

Limitations are fine, and eventually I even found this in the README (skipped over it on the first two reads because that sentence talks about eps and other detail, rather than telling me about the single most important limitation in the package). But surely the fact that the push! function just hangs on a point outside the [1,2] square is silly. Yes, it was my fault, but I just wasted half an hour updating every package and reinstalling everything to get rid of this, it didn't occur to me for a second that I am using the package incorrectly and thus it hangs...

juliohm commented 1 year ago

Closing the issue.