#5031, Kernel constructor cleanup #5036
Conversation
|
|
||
| private: | ||
| float64_t compute_recursive1(float64_t* avec, float64_t* bvec, int32_t len); | ||
| float64_t compute_recursive2(float64_t* avec, float64_t* bvec, int32_t len); | ||
|
|
||
| protected: | ||
| virtual float64_t compute(int32_t idx_a, int32_t idx_b); |
There was a problem hiding this comment.
minor, but might be worth leaving compute where it was? So that the class members are separate from the class functions?
|
Thanks! Are you going to push more kernel refactors here or shall I merge when it passes CI? |
src/shogun/kernel/ANOVAKernel.cpp
Outdated
| { | ||
| register_params(); | ||
| init(l, r); | ||
| ASSERT(DotKernel::init(l, r)); |
There was a problem hiding this comment.
I am curious, why is that changed?
There was a problem hiding this comment.
Just removed init(l,r) and pasted its code here. Also DotKernel::init(l,r) returns a bool, so I asserted that.
src/shogun/kernel/ANOVAKernel.h
Outdated
| * @return casted CANOVAKernel object, NULL if input was NULL | ||
| /** Casts the given kernel to ANOVAKernel. | ||
| * @param kernel Kernel to cast. Must be ANOVAKernel. Might be NULL | ||
| * @return casted ANOVAKernel object, NULL if input was NULL | ||
| */ | ||
| static std::shared_ptr<ANOVAKernel> obtain_from_generic(const std::shared_ptr<Kernel>& kernel); |
There was a problem hiding this comment.
I think these are not used anymore, so can be remove (see what happens if you do that)
There was a problem hiding this comment.
Makes sense, removed it.
There are a lot of kernels, some of them offer subtle complications. Also that I'm cleaning the docs too and the internal member names as well, so that'd be difficult for you guys to review(in case something bad goes down). I was instead planning on sending these in rounds. EDIT: I'll add a few more simple cases, I'll @ you when I'm done ? :) |
src/shogun/kernel/AUCKernel.h
Outdated
| { | ||
| return C_DENSE; | ||
| } | ||
| inline virtual const char* get_name() const { return "AUCKernel"; } |
There was a problem hiding this comment.
inline can be removed, it does nothing for us.
if you want to force use SG_FORCE_INLINE but I dont think that is needed here
There was a problem hiding this comment.
Yea, the compiler will do that for you since it’s such a small cost to inline it
src/shogun/kernel/LogKernel.h
Outdated
| protected: | ||
| /// distance to be used | ||
| std::shared_ptr<Distance> m_distance; | ||
| std::shared_ptr<Distance> m_distance = NULL; |
There was a problem hiding this comment.
No need to set a shared ptr to NULL, the default constructor sets the pointer to nullptr
There was a problem hiding this comment.
I modified the default constructor to do only SG_ADD. So I guess I'd need to set default values to internal members no?
There was a problem hiding this comment.
No need for shared ptr, it will by default own a nullptr
There was a problem hiding this comment.
But primitive types require initialisation yes, otherwise it’s undefined behaviour
| { | ||
| Kernel::init(l,r); | ||
| ASSERT(m_distance->init(l,r)); | ||
| init_normalizer(); |
There was a problem hiding this comment.
removing its init(l,r) method (used only inside LogKernel constructor and pasted the code here.
There was a problem hiding this comment.
Ok, but you need to keep the init(l,r) method so I guess just leave it there
There was a problem hiding this comment.
Do we absolutely need to? I removed init(l,r) from other classes as well.
There was a problem hiding this comment.
Yes
shogun/src/shogun/kernel/Kernel.h
Line 455 in 7014676
There was a problem hiding this comment.
Actually it isn’t pure virtual so I guess you don’t need to
There was a problem hiding this comment.
Very well, so I'll restore the init(l,r) methods. Maybe we can do something about them in future
There was a problem hiding this comment.
Actually it isn’t pure virtual so I guess you don’t need to
Yes but there are some methods (other than constructors) which are calling init(l,r).
| Kernel::init(l,r); | ||
| m_distance->init(l,r); | ||
| return init_normalizer(); | ||
| } |
There was a problem hiding this comment.
Deleting this is getting you the build error. This is implementing a pure virtual method from kernel
* ANOVA * AUC * Const * Log * DIag
|
Thanks! :) |
Ding dong! , did anyone order cleanup? #5030
@gf712