-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New package: Frechet Distance #8284
base: master
Are you sure you want to change the base?
Conversation
Still has to be completed.
Still not done, but at least a bit nicer.
This is just a starter to enable using the same types all over the package.
Has to be debugged now...
typename typeset_intersection<typename K1::Object_list, typename K2::Object_list>::type | ||
template<class K1, class K2, | ||
class List_= Default, | ||
typename NTc = NT_converter<typename Get_type<K1, FT_tag>::type, typename Get_type<K2, FT_tag>::type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC:
- You want to be able to convert from an approximate (Interval_nt) kernel to an exact kernel: can you explain in what conditions this is used? Are the intervals guaranteed to be points, or is any value in the interval ok for your purpose?
- NT_converter does not support conversion from Interval_nt to anything but double: if converting to something else makes sense, shouldn't we specialize NT_converter?
- Instead of passing a functor that converts to the rational type, you pass one that converts to double and rely on an implicit conversion happening afterwards. At first this feels like a hack, but problems seem unlikely (and it might even have slightly better performance than the "clean" solution for some kernels), so that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the use case the interval has inf == sup. And we can construct rationals from double
. I appreciate that you review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if the intervals are points, the conversion makes sense. To go back to my second point: what do you think of specializing NT_converter<Interval_nt,X>
so that
- it asserts that the interval is a point (CGAL_precondition?)
- it chains
NT_converter<Interval_nt,double>
(or actually calls.sup()
directly, no need to average 2 identical values) andNT_converter<double,X>
?
Adding a parameter to the kernel converter is not a bad idea, but I think it should only happen if we first reject the simpler solution (maybe you already did and I simply missed earlier discussions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I find the NT converter as template parameter cleaner than the specialization. Also I like that in dD it is like in 2D/3D.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is there any real use of that third parameter in 2D/3D? The only uses I found (not an exhaustive search) were with To_double or To_interval and they look like they would probably work just as well with the default parameter.
- Having a parameter does make it easier to have different versions, for instance this one that expects a trivial interval vs what I did in (the abandoned) Epack_d where I accepted conversions depending on
has_smaller_relative_precision
. So 👍 for the extra parameter. - I guess this just leaves the comment (similar to New package: Frechet Distance #8284 (comment)) about replacing
NT_converter<distance_t,double>
(as template parameter passed to the kernel converter) with something more specific (assert that it is a point, only use one of inf/sup) at some point in the future (for dim 2/3/d).
typedef typename Get_type<K1, FT_tag>::type FT1; | ||
typedef typename Get_type<K2, FT_tag>::type FT2; | ||
typedef NT_converter<FT1, FT2> NTc; | ||
NTc c; // TODO: compressed storage as this is likely empty and the converter gets passed around (and stored in iterators) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are motivated, you can replace this old comment with CGAL_NO_UNIQUE_ADDRESS. (you don't have to, I just happened to notice it while looking at the adjacent lines)
@@ -63,6 +64,12 @@ struct Cartesian_filter_K : public Base_ | |||
typedef typename Store_kernel<EK_>::reference_type EK_rt; | |||
EK_rt exact_kernel()const{return sek.kernel();} | |||
|
|||
|
|||
// Added to be similar to CGAL::Filtered_kernel | |||
typedef Kernel_d_interface<EK> Exact_kernel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to @mglisse : think if there is a better place to put this, using Kernel_d_interface at this low level doesn't look right. Maybe in Kernel_d_interface, I can check if there is a type Exact_kernel, and in that case override it with a typedef using Kernel_d_interface (recursively)?
…t a precomputed result
@@ -34,5 +38,50 @@ using Kernel = unspecified; | |||
*/ | |||
using Point = unspecified_type; | |||
|
|||
/*! The number type of the filtered kernel. If |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
? something missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better look at it once I ask for review. Sorry for these WIP commits.
/build:v1 |
The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8284/v1/Manual/index.html |
If I get it correctly |
/force-build:v1 |
The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8284/v1/Manual/index.html |
Yes. The package Triangulation defines for instance a concept DelaunayTriangulationTraits that is not implied by the Kernel_d concept, it has Ep[ie]ck_d as models but not Cartesian_d. |
Summary of Changes
Add a new package computing the approximate Fréchet distance between two polylines in dD under Euclidean metric, or decides if the distance is smaller than a given value.
Release Management
TODO: