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.

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

refactor: implement scaffold for finish_example split and POC migrations #4296

Merged
merged 15 commits into from Dec 5, 2022

Conversation

jackgerrits
Copy link
Member

No description provided.


if (_finish_example_fd.print_example_f == nullptr)
THROW("fatal: learner did not register print example fn: " + _name);
if (!has_legacy_print_example()) { THROW("fatal: learner did not register print example fn: " + _name); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

is print_example the legacy print? we should just remove it, its only cb_adf and cb_explore_adf_*

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it will be superseded by output/record and removed once that is done but until that happens I figured marking it legacy indicates its fate

@jackgerrits jackgerrits changed the title refactor: implement scaffold for finish_example split refactor: implement scaffold for finish_example split and POC migrations Dec 2, 2022
@@ -55,6 +55,22 @@ void VW::details::return_simple_example(VW::workspace& all, void*, VW::example&
VW::finish_example(all, ec);
}

void VW::details::record_stats_simple_label(
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe update_stats instead of record_stats? it is in sync with sd.update and it is clear that it has side effects on a shared object since record could just mean record the stats somewhere

Copy link
Member Author

Choose a reason for hiding this comment

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

update_stats is better I agree

if (ld.label != FLT_MAX && !ec.test_only) { sd.weighted_labels += (static_cast<double>(ld.label)) * ec.weight; }
}

void VW::details::output_example_simple_label(
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this function specific to the label?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is a general implementation for simple labels

@jackgerrits jackgerrits merged commit bdd4652 into VowpalWabbit:master Dec 5, 2022
@jackgerrits jackgerrits deleted the split_finish_scaffold branch December 5, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants