Change the centroids type to MatType instead of Mat<double> #3568
Conversation
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
rcurtin
left a comment
rcurtin
left a comment
There was a problem hiding this comment.
The changes here look good to me too, but I do think it's worth adding a very simple test for RangeSearch and DBSCAN just to ensure they can work with arma::fmat and float as ElemType. You could even adapt one of the simple existing tests from a one-type TEST_CASE() to a multi-type TEMPLATE_TEST_CASE().
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
|
@rcurtin Tests have been added, this one should be good to go and for review 👍 |
Signed-off-by: Omar Shrit <[email protected]>
rcurtin
left a comment
rcurtin
left a comment
There was a problem hiding this comment.
Thanks for adding the test---the changes are looking good. Just some small comments. I'm excited about properly supporting alternate matrix types for DBSCAN... this will make the documentation task much easier too. 😄
Signed-off-by: Omar Shrit <[email protected]>
rcurtin
left a comment
rcurtin
left a comment
There was a problem hiding this comment.
Looks good to me---just a couple small style comments. Hopefully we can fix FloatKDTree soon 😄
| //! Easy access to the MatType. | ||
| typedef typename RangeSearchType::Mat MatType; | ||
| //! Easy access to Element Type of the matrix. | ||
| typedef typename RangeSearchType::Mat::elem_type ElemType; |
There was a problem hiding this comment.
I'll make sure to write in the documentation that specifying MatType inside of RangeSearchType is the way to do it for DBSCAN... maybe it's a little awkward, but until we think of something better, it's better than having to specify MatType in two places. 👍
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
![mlpack-bot[bot]](https://avatars.githubusercontent.com/in/23934?s=60&v=4){kind=small}
2 participants
No description provided.