kde added #2383
kde added #2383
Conversation
mazumdarparijat
commented
Jul 13, 2014
mazumdarparijat
commented
- kde using single tree added
|
@iglesias Work for you when you are back! haha.. On a serious note now, please have a look at the PR. I have addressed your comments from the previous PR as well. Let me know if I have missed something. |
| EM_BALLTREE_SINGLE | ||
| }; | ||
|
|
||
| /** @brief This class implements kernel density estimation technique. Kernel density estimation is a non-parametric |
There was a problem hiding this comment.
"the kernel density estimation technique", or just "implements kernel density estimation".
|
Will continue looking tomorrow from kd-tree and below in the "files changed" tab. Now...ZzzZzz |
| float64_t min_distsq(bnode_t* node,float64_t* feat, int32_t dim); | ||
| float64_t min_dist(bnode_t* node,float64_t* feat, int32_t dim); | ||
|
|
||
| /** get min as well max distance of a node from a point |
|
@iglesias about inline functions, putting these in implementation files gives me |
| * faster than ball trees at lower dimensions. In case of high dimensional data, ball tree tends to out-perform KD-tree. | ||
| * By default, the class used Ball tree. | ||
| */ | ||
| class CKernelDensity : public CMachine |
There was a problem hiding this comment.
Ok, this is a somewhat more important comment as it concerns the design of this class. Did you check whether it makes sense to make KDE a subclass of CDistribution and put it under the distribution directory instead of machine? IIRC, this was the idea I had in mind when we wrote the GSoC project idea.
There was a problem hiding this comment.
@iglesias I can move this to CDistribution. In fact, this is a distribution indeed that we are trying to estimate. But, like here, there also I will be using just the train method from the interface. Others I will have to put as SG_NOTIMPLEMENTED?
There was a problem hiding this comment.
Does it make sense for the other [pure] virtuals to be implemented in the KDE class?
There was a problem hiding this comment.
No, I think not!
|
@iglesias updated! |
|
@iglesias I updated the remaining comments. I also added dual distances which I was working on. Just 2 small methods are added. If you find it difficult to review with this PR, let me know. I can to remove them and then send the update. |
|
No problem about the dual distances, as you mentioned, they are simple enough. Coming back to the inline-ness, I think it is explained here pretty well: http://www.parashift.com/c++-faq/where-to-put-inline-keyword.html. I found really fun to read the NOTE at the bottom :-D |
|
I could get gigged for my definition-declaration terminology! Eye opener really! ;-) |
|
@iglesias have a look at the travis make error. I was talking about this error only. For some reason, locally this no longer happens. |
|
To speed things up a bit, I'm going back to putting the inline code in the header file. ../../../src/shogun/libshogun.so.16.1: undefined reference to `shogun::CNbodyTree::actual_dists(double)' |
|
Just restarted the build Parijat. Let me know if it works well this time On 16 July 2014 08:37, Parijat Mazumdar notifications@github.com wrote:
|
|
@iglesias For some reason travis build is not working on the last commit. It might be waiting in a queue though. |
|
@iglesias I think this can be merged now. The travis error is unrelated (and appears in all other PRs as well. Something wrong got merged maybe!) |
|
Your are right. It seems unrelated to this PR definitely. Merging this one then. Nice one, @mazumdarparijat! Looking forward to seeing the notebook showcasing KDE :-) |
|
@iglesias Before the notebook, I have to complete the dual tree algorithm for KDE. |
