[mlpack-git] [mlpack] Fix bug #358 (CosineTreeTest/CosineNodeCosineSplit test sometimes fails, only on i386) (#581)

[Ryan Curtin]

Okay, I see what you mean.

So I spent a long time thinking about it, and the paper does not consider floating point issues.  I think that when Michael Holmes suggested to ignore points whose cosine to the given point held by the node is 1, he was intending to ignore points that were scaled versions of the point held by the node.  But in reality we are ignoring points that are the scaled versions of the point held by the node or that have extremely close cosines to that point.

I did not do a floating-point analysis to see how much error is introduced into the tree by ignoring all points with cosines (1 - epsilon/2) instead of all points with cosine 1, and I really don't want to do that analysis.  So I'm willing to just take it on faith that any errors introduced by this problem later in the algorithm will be sufficiently small that we don't need to worry about it.

I agree with your fix, but I've added two minor comments.  Let me know what you think and make any changes if you like, then I'll go ahead and merge.  If you want to fix the code to adhere to the style guidelines (https://github.com/mlpack/mlpack/wiki/DesignGuidelines#style-guidelines) please feel free, otherwise I will do that after the merge and send a link to the diff.

Thanks again for looking into this!  I did not think I would ever see this one solved.  If I ever run into you in real life, I owe you a drink. :)

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/mlpack/mlpack/pull/581#issuecomment-200388437
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.cc.gatech.edu/pipermail/mlpack-git/attachments/20160323/c818642a/attachment.html>