Quick hull #1539
Quick hull #1539
Conversation
|
All done ! |
JacquesOlivierLachaud
added
ready for review 👍
and removed
waiting author feedbacks
labels
| // } | ||
|
|
||
| // #ifdef WITH_BIGINTEGER | ||
| // /// @param i any integer |
There was a problem hiding this comment.
why this code is commented? not working?
There was a problem hiding this comment.
sorry I have to clean this part. I moved it into kernel
| /// Allows seamless conversion of integral types and lattice | ||
| /// points, while checking for errors when going from a more | ||
| /// precise to a less precise type. | ||
| /// |
There was a problem hiding this comment.
For all this, why not relying on NumericalTraits?
There was a problem hiding this comment.
Maybe this is introducing duplicate codes/features
There was a problem hiding this comment.
Because they are not sufficient for this purpose.
In fact gmp cannot initialize a big int from an int64 variable (but can from a const expression).
Conversely, gmp can do some conversion with long (which may or may not be 64 bits), but not on long long.
Finally, I wish to print warnings when bad conversions occur.
There was a problem hiding this comment.
should we remove the NumberTraits converter and use this one?
There was a problem hiding this comment.
Perhaps we have to make more checks before doing that. Anyway the numbertraits converter just converts to int64. I was afraid that it was incorrect for BigInteger but it was working on my mac (where long is also 64 bits). So perhaps it is no harm keeping it. Otherwise, a cleaner IntegerConverter would have a "status", instead of displaying warnings. We can perhaps make an issue and I do a small PR afterwards to do that. What do you think ? Among other limitations, I only treat the signed case, and I do not care about small integers.
There was a problem hiding this comment.
I don't know.. I am afraid that having converters at several location could be a problem (like the dozen of functors/classes to threshold an image). For me, NumberTraits is a good place to contain such helpers/converters. Let me check again the converters you have here...
| /// Allows seamless conversion of integral types and lattice | ||
| /// points, while checking for errors when going from a more | ||
| /// precise to a less precise type. | ||
| /// |
There was a problem hiding this comment.
Maybe this is introducing duplicate codes/features
|
I'd need to clone locally. Is the new version working on big integers without floating point computations? |
|
The new version normally works with int32, int64, BigInteger (coordinate or internal integers). |
|
Should be ok now ;-) |
|
Thx! |
|
🎉🎉🎉 |
|
I think we are good for the release (plenty of new stuff + python) |
|
Yes. DGtal is very alive ! |
PR Description
This Pull Request provides an implementation of the Quick Hull convex hull algorithm. It works in arbitrary dimension. It provides several kernels to deal with lattice or rational points, and also to compute the Delaunay cell complex.
Checklist
cmakemode (otherwise, Travis C.I. will fail).