Skip to content
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

Jump to bottom

feat: IGL reduction #4295

Merged
merged 21 commits into from
Apr 5, 2023
Merged

feat: IGL reduction #4295

merged 21 commits into from
Apr 5, 2023

Conversation

cheng-tan
Copy link
Collaborator

@cheng-tan cheng-tan commented Dec 1, 2022 •
edited
Loading

  • Added CB_WITH_OBSERVATIONS label type for IGL problems
  • In json parser, if the label type is CB_WITH_OBSERVATIONS, "o" will be parsed and added as the last example (observation example) when constructing vw examples
  • Updated shared feature merger to not merge observation example
  • Update learn/predict in IGL reduction to use inverse kinematics approach
  • Updated stats print in IGL reduction

@cheng-tan cheng-tan changed the title WIP: IGL reduction [wip] IGL reduction Dec 1, 2022
@cheng-tan cheng-tan marked this pull request as draft December 1, 2022 14:56
@cheng-tan cheng-tan force-pushed the igl-reduction-cleanup branch 3 times, most recently from a00ff2a to a7ab250 Compare December 6, 2022 16:19
jackgerrits
jackgerrits requested changes Dec 6, 2022
View reviewed changes
test/unit_test/igl_test.cc Outdated Show resolved Hide resolved
vowpalwabbit/core/src/reductions/gd.cc Outdated Show resolved Hide resolved
vowpalwabbit/core/src/reductions/shared_feature_merger.cc Outdated Show resolved Hide resolved
vowpalwabbit/core/src/reductions/shared_feature_merger.cc Outdated Show resolved Hide resolved
vowpalwabbit/core/src/reductions/shared_feature_merger.cc Outdated Show resolved Hide resolved
vowpalwabbit/core/src/reductions/interaction_ground.cc Outdated Show resolved Hide resolved
vowpalwabbit/core/src/reductions/interaction_ground.cc Outdated Show resolved Hide resolved
vowpalwabbit/core/src/reductions/interaction_ground.cc Outdated
multi_learner *pi_learner = as_multiline(stack_builder.setup_base_learner());

// 1. fetch already allocated coin reduction
auto* ftrl_coin = pi_learner->get_learner_by_name_prefix("ftrl-Coin");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Provide a better error message if the user didn't supply coin.

vowpalwabbit/core/src/reductions/interaction_ground.cc Outdated Show resolved Hide resolved
vowpalwabbit/core/src/reductions/interaction_ground.cc Outdated Show resolved Hide resolved
@lalo lalo changed the title [wip] IGL reduction feat: [wip] IGL reduction Jan 5, 2023
@cheng-tan cheng-tan force-pushed the igl-reduction-cleanup branch 6 times, most recently from f3080de to efc10bb Compare January 24, 2023 16:40
@cheng-tan cheng-tan force-pushed the igl-reduction-cleanup branch 2 times, most recently from b6f6217 to dddf73a Compare January 24, 2023 20:55
@cheng-tan cheng-tan force-pushed the igl-reduction-cleanup branch 2 times, most recently from 3e81c3b to c764e99 Compare February 3, 2023 22:35
@cheng-tan cheng-tan marked this pull request as ready for review March 1, 2023 00:56
@cheng-tan cheng-tan force-pushed the igl-reduction-cleanup branch 3 times, most recently from 9f0d953 to 7492dbd Compare March 1, 2023 02:12
@cheng-tan cheng-tan changed the title feat: [wip] IGL reduction feat: IGL reduction Mar 1, 2023
@cheng-tan cheng-tan force-pushed the igl-reduction-cleanup branch 2 times, most recently from 29dcc42 to 220af1d Compare March 2, 2023 16:59
jackgerrits
jackgerrits reviewed Apr 5, 2023
View reviewed changes
vowpalwabbit/core/include/vw/core/reductions/ftrl.h Outdated
#include <cstddef>
#include <cstdint>

namespace
Copy link
Member

Choose a reason for hiding this comment

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

Anonymous namespaces should not be used in headers. This should be in a namespace like VW::reductions

jackgerrits
jackgerrits reviewed Apr 5, 2023
View reviewed changes
vowpalwabbit/core/src/cb_with_observations_label.cc Outdated
VW::label_parser_reuse_mem& /*reuse_mem*/, const std::vector<VW::string_view>& /*words*/,
VW::io::logger& /*logger*/)
{
// TODO: implement text format parsing for cb with observations
Copy link
Member

Choose a reason for hiding this comment

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

If this is not implemented please make it throw so a user knows it not there

@cheng-tan cheng-tan merged commit 6b0ff6e into master Apr 5, 2023
@cheng-tan cheng-tan deleted the igl-reduction-cleanup branch April 5, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bassmang bassmang bassmang left review comments

@byronxu99 byronxu99 byronxu99 left review comments

@jackgerrits jackgerrits jackgerrits approved these changes

@lalo lalo lalo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cheng-tan @lalo @jackgerrits @byronxu99 @bassmang