-
Notifications
You must be signed in to change notification settings - Fork 917
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
Refactor/forecast residuals fn #1223
Conversation
- past and future covariates in residuals()
- past and future covariates in residuals()
…d CONTRIBUTING.md)
…d and CONTRIBUTING.md)
…STALL.md and CONTRIBUTING.md)
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.
Looks good 👍 I only added a suggestion to add one more unit test (testing the actual residual computations with covariates).
series._assert_univariate() | ||
try: | ||
series._assert_univariate() | ||
except (AttributeError, TypeError): |
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.
👍
@@ -0,0 +1,75 @@ | |||
import numpy as np |
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.
+1 for the separate test file
darts/tests/utils/test_residuals.py
Outdated
past_covariates=past_covariates, | ||
future_covariates=future_covariates, | ||
) | ||
# it seems that models can be fit with data objects that are not necessarily TimeSeries, but residuals() will |
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.
The only other possible type should be Sequence[TimeSeries]
😅
target_series, | ||
past_covariates=past_covariates, | ||
future_covariates=future_covariates, | ||
) |
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.
Could you also add a test case where the residuals computation carries out successfully with past and/or future covariates?
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.
ok will do
Some of the unit tests fail, but this is because #1228 |
Codecov ReportBase: 93.71% // Head: 93.70% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1223 +/- ##
==========================================
- Coverage 93.71% 93.70% -0.01%
==========================================
Files 83 83
Lines 8416 8410 -6
==========================================
- Hits 7887 7881 -6
Misses 529 529
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…into refactor/forecast_residuals_fn
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.
LGTM, nice :)
Fixes #1127.
Summary
Other Information