Simplify min/sort comparisons

mourner / delaunator-rs

Fast 2D Delaunay triangulation in Rust. A port of Delaunator.

https://docs.rs/delaunator

ISC License

207 stars 28 forks source link

Simplify min/sort comparisons #6

Closed RReverser closed 6 years ago

RReverser commented 6 years ago

Use ordered-float crate that provides strong ordering on floats by ignoring IEEE.754 spec in terms of handling NaN (which is what we did anyway). This allows to use *_by_key functions for comparisons on iterables.
Change public API to return Option, so that when triangulation doesn't exist, None would be returned instead of panicking (this is more idiomatic in Rust).

mourner commented 6 years ago

BTW, I just added the tests — let's rebase.

mourner commented 6 years ago

Hmm, cargo bench shows around 2-6% slowdown...

mourner commented 6 years ago

So, it seems like there's no perf benefit in the PR, so I'd prefer to keep the package dependency-less, so maybe let's close? I added idiomatic error handling + tests for them in https://github.com/mourner/delaunator-rs/commit/66f46d31e13e2521b0d0436e78885b2b9b7d0979

RReverser commented 6 years ago

Let me check again on Monday, don't close yet. In theory perf shouldn't have changed.

mourner commented 6 years ago

@RReverser maybe it's just noise and the perf is unchanged, but I'm still hesitant because I'm not yet convinced in the benefit — the code doesn't get much simpler, but introduces a dependency.

RReverser commented 6 years ago

It does get cleaner IMO, and usually these built in methods have specialisations and potential to get faster (and also easy integration with various crates to make them easily eligible for multithreading, SIMD etc.).

mourner commented 6 years ago

OK, let's rebase and look at benchmarks again :)

mourner commented 6 years ago

Closing as stale for now.