Skip to content

refactor: Refactor cats_tree.cc to use builder to set features of learner #3053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bassmang merged 8 commits into from
Jun 10, 2021
Merged

refactor: Refactor cats_tree.cc to use builder to set features of learner #3053

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4e8d79b
refactor: Refactor cats_tree.cc to use builder to set features of lea…
bassmang Jun 8, 2021
dad9ba5
Merge remote-tracking branch 'upstream/master' into refac
bassmang Jun 8, 2021
d36443e
initialize all variables in cats_tree
bassmang Jun 9, 2021
b7bf12a
Merge branch 'master' into refac
bassmang Jun 9, 2021
f137889
Use 2 constructors
bassmang Jun 10, 2021
3bf8b0b
Merge remote-tracking branch 'upstream/master' into refac
bassmang Jun 10, 2021
5b4f0f1
Merge branch 'refac' of https://github.com/bassmang/vowpal_wabbit int…
bassmang Jun 10, 2021
9e62635
Merge branch 'master' into refac
bassmang Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions vowpalwabbit/cats_tree.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "explore_internal.h"
#include "hash.h"
#include "guard.h"
#include "label_parser.h"

using namespace VW::config;
using namespace VW::LEARNER;
Expand Down Expand Up @@ -360,16 +361,18 @@ base_learner* setup(options_i& options, vw& all)
options.replace("link", "glf1");
}

auto tree = scoped_calloc_or_throw<cats_tree>();
auto tree = VW::make_unique<cats_tree>();
Copy link
Member

@jackgerrits jackgerrits Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like _cost_star, _a, _b are unitialized after this call.

Copy link
Member Author

@bassmang bassmang Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just recommitted. Let me know if manually initializing _a and _b is preferred to creating a constructor in node_cost.

Copy link
Member

@jackgerrits jackgerrits Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the node_cost initialization done internally is good as it affects any other usage too. So this approach is better

tree->init(num_actions, bandwidth);
tree->set_trace_message(all.trace_message.get(), all.logger.quiet);

base_learner* base = setup_base(options, all);

learner<cats_tree, example>& l = init_learner(tree, as_singleline(base), learn, predict, tree->learner_count(),
prediction_type_t::multiclass, all.get_setupfn_name(setup));

return make_base(l);
int32_t params_per_weight = tree->learner_count();
auto* l = make_reduction_learner(std::move(tree), as_singleline(base), learn, predict, all.get_setupfn_name(setup))
.set_params_per_weight(params_per_weight)
.set_prediction_type(prediction_type_t::multiclass)
.set_label_type(label_type_t::cb)
.build();
return make_base(*l);
}

} // namespace cats_tree
Expand Down
3 changes: 2 additions & 1 deletion vowpalwabbit/cats_tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ struct node_cost
{
uint32_t node_id;
float cost;
node_cost(uint32_t node_id = 0, float cost = 0) : node_id(node_id), cost(cost) {}
Copy link
Member

@jackgerrits jackgerrits Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member Author

@bassmang bassmang Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually tried this initially, however it causes a compilation error with commands such as "_a = {node_id, _cost_star};"

Copy link
Member

@jackgerrits jackgerrits Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. In that case I think we should have two constructors. Default and two parameter rather than a since one with default parameters.

Copy link
Member Author

@bassmang bassmang Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok just recommitted.

};

struct cats_tree
Expand All @@ -72,7 +73,7 @@ struct cats_tree
uint64_t app_seed = uniform_hash("vw", 2, 0);
std::string tree_stats_to_string();
min_depth_binary_tree _binary_tree;
float _cost_star;
float _cost_star = 0;
Copy link
Member

@jackgerrits jackgerrits Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a float literal 0.f

node_cost _a;
node_cost _b;
std::ostream* _trace_stream = nullptr;
Expand Down