-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: move LabelDict namespace items into other namespaces, add const #4245
refactor: move LabelDict namespace items into other namespaces, add const #4245
Conversation
@@ -35,6 +35,12 @@ namespace VW | |||
void copy_example_data(example* dst, const example* src); | |||
void setup_example(VW::workspace& all, example* ae); | |||
|
|||
void add_example_namespace(VW::example& ec, VW::namespace_index ns, const features& fs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we try to follow std naming conventions at some point? insert/erase instead of add/del?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions are already a bit spooky because the del is actually just a truncate based on the size of the given features. Renaming to insert/erase seems reasonable though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think append and truncate better indicate what exactly they do, especially since you cannot give a position to insert at
vowpalwabbit/core/src/example.cc
Outdated
@@ -15,6 +15,58 @@ | |||
#include <climits> | |||
#include <cstdint> | |||
|
|||
void VW::del_example_namespace(VW::example& ec, VW::namespace_index ns, const features& fs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a function that should be exposed directly to users via the public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this seems like a good candidate for the details namespace
9a4a746
to
eea0c51
Compare
No description provided.