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

Fix: Using gridsearch with use_fitted_values=True raises unexpected error #2222

Merged
merged 9 commits into from
Feb 26, 2024

Conversation

madtoinou
Copy link
Collaborator

Summary

When using gridsearch() with use_fitted-values=True, the model is not properly created to check if it indeed has the attribute fitted_values (due to incorrectly assuming that the class had default arguments).

from darts.models import NHiTSModel
import darts.utils.timeseries_generation as tg

ts2 = tg.sine_timeseries(length=25, column_name="dummy_ts_2") + 100

# also fail for regression models
model = NHiTSModel.gridsearch(
    parameters={
        "input_chunk_length":[9],
        "output_chunk_length":[2,3],
    },
    series=ts2,
    use_fitted_values=True
)

>>> TypeError: init() missing 2 required positional arguments: 'input_chunk_length' and 'output_chunk_length

Other Information

Added a check to ensure that each values in the parameters dictionary is indeed a list otherwise the itertools.product will fail.

@madtoinou madtoinou added the bug Something isn't working label Feb 8, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8073de4) 93.88% compared to head (6e1c451) 93.85%.

Files Patch % Lines
darts/models/forecasting/forecasting_model.py 50.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2222      +/-   ##
==========================================
- Coverage   93.88%   93.85%   -0.03%     
==========================================
  Files         135      135              
  Lines       13429    13419      -10     
==========================================
- Hits        12608    12595      -13     
- Misses        821      824       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Thanks @madtoinou for this 🚀

Should we allow for np arrays as well for the params values?

Also this introduces breaking changes that we need to list in the changelog:

  • will raise an error now if params values are not list (or np ndarray)

darts/models/forecasting/forecasting_model.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot @madtoinou 🚀

@dennisbader dennisbader merged commit ec53511 into master Feb 26, 2024
9 checks passed
@dennisbader dennisbader deleted the fix/bug_grisearch branch February 26, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants