-
Notifications
You must be signed in to change notification settings - Fork 3
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
Renames and unit tests #20
Conversation
@@ -7,13 +7,22 @@ | |||
import glob | |||
import logging | |||
|
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.
this file change just has black
formatting
rl_chain/__init__.py
Outdated
from .pick_best_chain import ( | ||
PickBest, | ||
PickBestAutoSelectionScorer, | ||
PickBestFeatureEmbedder, |
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.
maybe we should not expose embedders from top level?
- inheritance from them seems quite advanced scenario, so they may confuse
- not clear why inheritance from them may be needed since they have almost no state and only one function
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.
yeah I can remove
consistent naming
TextEmbedder
->FeatureEmbedder
andto_vw_format
->format
score/scoring
for anything reward relatedactions/context
renamed toto_select_from/based_on
label
andcost/action/probability
renamed toSelection
for theLabel
class and then the variable is calledselected
in the event, which makes it nice:selected.score
,selected.index
,selected.probability