-
Notifications
You must be signed in to change notification settings - Fork 863
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 Develop #156
Fix Develop #156
Conversation
…FourTheta doc(gridsearch): correct type in doc
* Add normalization choice * Add comment to be clearer * Correct the docs * clean the code and add a check on mean=0
This reverts commit 47f16b8.
darts/backtesting/backtesting.py
Outdated
@@ -268,7 +268,7 @@ def forecasting_residuals(model: ForecastingModel, | |||
first_index = series.time_index()[model.min_train_series_length] | |||
|
|||
# compute fitted values | |||
p = backtest_forecasting(series, model, first_index, fcast_horizon_n, True, verbose=verbose) | |||
p = backtest_forecasting(series, model, first_index, fcast_horizon_n, trim_to_series=True, verbose=verbose) |
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.
should this be a part of this PR?
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 is a part of fixing the develop branch after searching the cause of bugs. But this is not caused by the PR #123
So yes, it doesn't belong to this PR
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.
Wait so where exactly did this cause a bug? Because shouldn't line 274 make sure that the two series have the same indices anyways? I mean I think the change is good, because it should be (very) slightly more efficient. But I believe it would be good to have clearer scopes in individual PRs (don't get me wrong though, I'm guilty of this too).
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.
My bad, there is no bug, because this function is only for univariate time series. I revert it.
Nonetheless, this parameter shouldn't be here, even if it does nothing for now.
.github/workflows/merge.yml
Outdated
|
||
jobs: | ||
tests: | ||
runs-on: ${{ matrix.os }} | ||
runs-on: macos-latest |
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.
following the ticket from JIRA we agreed to:
"Run notebooks+unit tests+mac tests on PR and only unit tests on push"
shouldn't this workflow run all the test then and there should be a separate workflow for the push to the feature branch?
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.
That was what I had before e77e94f
The problem is that on push and on pull-request triggers at the same time and we have duplicate tests.
I propose this solution to keep the chosen behavior
(PR and develop workflow run together on push devevelop and pull-request on develop, but on push on other branches, only develop workflow runs)
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 didn't find a way to execute only the PR workflow on pull-request.
So if duplicate tests isn't bothering anyone, I can revert this.
What do you think?
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 can be acceptable in the beginning to have second send of tests running during PR, as long as we don't run all the test suites on every push (it would take too long to execute).
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.
IMO we should have every test that is needed in pull-requests
otherwise the tests that are 'exclusive' only for push won't run on pull requests from forks. Like right now no tests were run on two prs from contributors.
This reverts commit f2843c0.
darts/tests/test_4theta.py
Outdated
@@ -50,7 +50,7 @@ def test_theta(self): | |||
fourtheta.fit(series) | |||
forecast_theta = theta.predict(20) | |||
forecast_fourtheta = fourtheta.predict(20) | |||
self.assertTrue((forecast_theta - forecast_fourtheta <= 1e-12).all()[0]) | |||
self.assertTrue((forecast_theta - forecast_fourtheta <= 1e-10).all()[0]) |
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.
Did you test whether the output of the models is deterministic in this context?
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.
You are right, the output is not deterministic, thus explaining the problem with one of the test (the precision error was doubled)
Is it possible to open two separate PRs: one for the workflows and one for the changes to the model/backtesting? |
This reverts commit 03f2ffd.
…o feat/FourTheta
damped: Optional[bool] = False, | ||
seasonal: Optional[str] = 'additive', | ||
seasonal: Optional[ModelMode] = ModelMode.ADDITIVE, |
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.
Are the underlying string values of the enum still supported as argument values?
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, the raw strings are not suported anymore. We only accept the Enum members.
But to be backward-compatible, it can be interesting to accept both.
Fix PR #123
Summary
Correct Bugs from PR #123