Skip to content
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

change order of arguments for metric in gridsearch #989

Merged
merged 9 commits into from Jun 21, 2022

Conversation

ClaraGrthns
Copy link
Contributor

Fixes #979.

Summary

Metric in gridsearch method of the forecasting class now receives correct order of arguments: First the actual series, second the predicted/fitted series.

Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!
Could you maybe change the docstring of the gridsearch and backtest methods to make this clear?

Something like

metric
            A function that takes two TimeSeries instances as inputs (actual and prediction, in this order), and returns a float error value.

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #989 (ea86f6e) into master (a1328fa) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #989   +/-   ##
=======================================
  Coverage   93.04%   93.04%           
=======================================
  Files          77       77           
  Lines        7861     7861           
=======================================
  Hits         7314     7314           
  Misses        547      547           
Impacted Files Coverage Δ
darts/models/forecasting/forecasting_model.py 96.77% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1328fa...ea86f6e. Read the comment docs.

@hrzn
Copy link
Contributor

hrzn commented Jun 17, 2022

@ClaraGrthns are you still willing to help us close this PR? We can do it as soon as the docstrings are updated.

@ClaraGrthns
Copy link
Contributor Author

Yes, I am sorry! I will do it next Tuesday!

Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ClaraGrthns
Do you agree to be mentioned / acknowledged in the changelog?

@hrzn
Copy link
Contributor

hrzn commented Jun 21, 2022

@ClaraGrthns the new docstring line is too long, which causes the linting checks to fail. Could you break the line? Thanks.

@ClaraGrthns
Copy link
Contributor Author

ClaraGrthns commented Jun 21, 2022

Just added the line break.

Thank you @ClaraGrthns Do you agree to be mentioned / acknowledged in the changelog?

yes, sure!

@hrzn hrzn merged commit b530dcd into unit8co:master Jun 21, 2022
@ClaraGrthns ClaraGrthns deleted the fix-gridsearch branch June 22, 2022 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Gridsearch: Error metrics in split window mode - misleading order of method parameters.
3 participants