-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add Date and Math Tasks #88
Conversation
…or/prompting into features/task-expansion
@bkb2135 do we have experimental results for this? |
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
Co-authored-by: Steffen Cruz <[email protected]>
We have wandb data (although we only have a single miner). I'm going to restart the run pulling the suggested changes, then I'll post some data |
# If the year is a digit, return the parsed date and the year in a tuple | ||
return (parsed_date, year) | ||
else: | ||
raise ValueError |
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.
No need to change but it seems that this raise ValueError
could be a continue
. Exceptions should be used for handling error conditions or unexpected situations, not for controlling normal flow of a program.
return score | ||
ref_date = self.parse_dates_from_text(reference) | ||
comp_date = self.parse_dates_from_text(completion) | ||
score =np.exp(-(self.date_diff(ref_date, comp_date)**2/1000)) |
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.
No need to add but a small comment explaining the rationale behind this formula could be appreciated for future maintainers
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.
It seems that this single file is testing multiple features (FloatDiff, DateReward
). Ideally, we have unit tests by feature so it's easier to track and understand for future maintainers. Variable naming is also something that could be improved, but I know sometimes it's hard to come up with good representative names for the variables we create.
Still talking about parametrization, I think the approach of defining the used variable in the decorator is more explicit than defining variables globally to be reused.
Last but not least, it seems that there are unused references in this test, such as
DiffRewardModel, RelevanceRewardModel, RougeRewardModel, RewardPipeline
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 added a couple of suggestions around improving code quality but the core of the functionality seems to be there
No description provided.