-
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
feat(CB_GF): correct update rule and simulation unit test #4565
Conversation
olgavrou
commented
Apr 18, 2023
- allow multiple costs if graph feedback
@@ -105,6 +105,7 @@ class cb_explore_adf_base | |||
const multi_ex& ec_seq, VW::io::logger& logger); | |||
|
|||
ExploreType explore; | |||
static bool allow_multiple_costs; |
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.
Does this need to be static?
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.
yes since it is used in predict/learn which are static
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.
If there are two workspace instances in the same process, one using cb with graph feedback and one not using it then this static would essentially break that scenario.
If this needs to be dynamic based on the setup then the variable and learn/predict should be non static
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.
actually it doesn't need to be static and good point, updated