-
Notifications
You must be signed in to change notification settings - Fork 531
fix validate_and_prepare_single_dict_task by separating "past only" and "future known" cov keys #344
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
fix validate_and_prepare_single_dict_task by separating "past only" and "future known" cov keys #344
Conversation
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.
@HarvestStars Thanks a lot for catching and fixing this! Left a few minor comments. Could you please address those? I will also run some evals on my side before merging to ensure that we did not break anything accidentally.
|
Hi @abdulfatir |
|
@HarvestStars Thanks that looks good to me. Could you also please format the code? You can do that by running. uvx ruff format src |
|
I re-ran the evals and numbers match. In any case, this issue mainly affects fine-tuning. Thank you for finding this and please feel free to open other issues you encounter with fine-tuning. We have not rigorously tested fine-tuning support yet, that's why it has been marked as experimental. |
|
Hi @abdulfatir |
226ebd5 to
a673a12
Compare
Closes #345
Hi @abdulfatir
Here is the bugfix about the function "validate_and_prepare_single_dict_task", which had 2 issue points:
task_n_future_covariates = len(task_future_covariates_list)astask_future_covariates_listis filled by forkey in task_covariates_keysSo, this PR fixed them by separating "past only" and "future known" covs from the "past_covariates" input, and explicitly put the "past only" covs rows above "future known" cov rows, supported by a temp list "ordered_covariate_keys".