BinarySpaceTree<BallBound<> > fails #307

Closed
rcurtin opened this Issue Dec 29, 2014 · 3 comments

Projects

None yet

1 participant

@rcurtin
Member
rcurtin commented Dec 29, 2014

Reported by rcurtin on 21 Oct 44095162 07:36 UTC
It is not possible to instantiate the class BinarySpaceTree<BallBound<> >, because the BallBound class does not have a Metric() function, but BinarySpaceTree has a Metric() function that attempts to access BallBound::Metric().

The solution to this is to have BallBound hold an instantiated metric as a member and to ensure that any calculations done with MetricType are done with the instantiated metric and not MetricType::Evaluate().

Until this is resolved it is not possible to use ball trees for any mlpack algorithm.

@rcurtin rcurtin self-assigned this Dec 29, 2014
@rcurtin rcurtin added this to the mlpack 1.0.9 milestone Dec 29, 2014
@rcurtin rcurtin closed this Dec 29, 2014
@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by rcurtin on 4 Dec 44142911 12:51 UTC
Hi,

Thank you for the patch, but it needs a little bit of work. A ball should work with any metric, not just the LMetric; after all, a ball with center c and radius r is defined as the set of points

{ p : d(p, c)  r }

for any metric d(). So instead of having Power and TakeRoot as a template parameter, the MetricType itself should be a template parameter, and all distance calculations done by the BallBound should be done by an instantiated MetricType. If you can make these modifications then we can start to integrate the patch.

Thanks,

Ryan

@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by yashdv on 26 Apr 44472485 05:13 UTC
I have done the essential changes to the API. However, we might need to test how the different metrics work with BallBound.

Also, we need to see if BinarySpaceTree<BallBound<> > works correctly.

@rcurtin
Member
rcurtin commented Dec 30, 2014

Commented by rcurtin on 23 Aug 44565967 07:28 UTC
I have committed the code in r16854 and r16855. Thanks Yash for the contribution! This also resulted in opening the somewhat related bug #356.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment