refactor: Refactor cats_tree.cc to use builder to set features of learner #3053
Conversation
| } | ||
|
|
||
| auto tree = scoped_calloc_or_throw<cats_tree>(); | ||
| auto tree = VW::make_unique<cats_tree>(); |
There was a problem hiding this comment.
It looks like _cost_star, _a, _b are unitialized after this call.
There was a problem hiding this comment.
Just recommitted. Let me know if manually initializing _a and _b is preferred to creating a constructor in node_cost.
There was a problem hiding this comment.
Having the node_cost initialization done internally is good as it affects any other usage too. So this approach is better
vowpalwabbit/cats_tree.h
Outdated
| std::string tree_stats_to_string(); | ||
| min_depth_binary_tree _binary_tree; | ||
| float _cost_star; | ||
| float _cost_star = 0; |
There was a problem hiding this comment.
Should be a float literal 0.f
vowpalwabbit/cats_tree.h
Outdated
| { | ||
| uint32_t node_id; | ||
| float cost; | ||
| node_cost(uint32_t node_id = 0, float cost = 0) : node_id(node_id), cost(cost) {} |
There was a problem hiding this comment.
This is nit, so feel free to wontfix. I'd say updating lines 55 and 56 is preferable as default params in constructors is a but confusing and the default constructor compared to this constructor with default parameters is ambiguous. (well default won't be generated in this case, and I think default default constructor is usually best)
uint32_t node_id = 0;
float cost = 0.f;There was a problem hiding this comment.
I actually tried this initially, however it causes a compilation error with commands such as "_a = {node_id, _cost_star};"
There was a problem hiding this comment.
I see. In that case I think we should have two constructors. Default and two parameter rather than a since one with default parameters.
2 participants
No description provided.