-
Notifications
You must be signed in to change notification settings - Fork 981
Fix/max_samples_per_ts #2987
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/max_samples_per_ts #2987
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2987 +/- ##
==========================================
- Coverage 95.63% 95.56% -0.07%
==========================================
Files 153 153
Lines 16433 16435 +2
==========================================
- Hits 15715 15706 -9
- Misses 718 729 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@brunnedu |
|
@brunnedu Hi, do you know when could this be merged? I also have a TimesFM PR that would need a review. |
|
Hi @daidahao, as @dennisbader, our codeowner, is currently away on leave, it will likely be another 2 weeks or so before we can merge these PRs. Regarding the TimesFM PR, I’d like @dennisbader to give it a final look once he’s back, as his familiarity with foundation models will be really valuable there. Thanks for your contributions! |
jakubchlapek
left a comment
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.
small comment on the changelog, but LGTM :) thanks
Co-authored-by: Jakub Chłapek <147340544+jakubchlapek@users.noreply.github.com>
dennisbader
left a comment
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.
Beautiful PR, thanks a lot @brunnedu 🚀
Just as a note to @daidahao: for multi-series of different lengths this will still upsample shorter series to have the same number of samples for each series (according to max samples of the longest series).
We can complete this PR once #2995 has been merged :)
Checklist before merging this PR:
Fixes #2986.
Summary
This PR fixes a bug in
ShiftedTorchTrainingDataset(and its subclassesSequentialTorchTrainingDatasetandHorizonBasedTorchTrainingDataset) where themax_samples_per_tsparameter was not properly acting as an upper bound on the number of samples extracted per time series.Example from issue:
Changes made:
ShiftedTorchTrainingDataset.__init__()to capmax_samples_per_tsat the maximum extractable samples over all series.test_max_samples_per_ts_upper_bound) that verifies:max_samples_per_ts=Nonemax_samples_per_ts > actual_max(the bug case)max_samples_per_ts < actual_maxstride > 1Thanks to @daidahao for identifying and reporting this issue!