-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Ts scikit models #1198
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
Ts scikit models #1198
Conversation
Sorry, don't review for now. I will merge with @TremaMiguel 's eventual PR and then you can review it. |
#### CLASSICAL STATISTICAL MODELS #### | ||
###################################### | ||
|
||
|
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.
To make more clear and clean the __init__
of the ArimaContainer, is it possible to decompose this process into methods that get called in the __init__
?. For example return_order_related_params
can be a method of this class called during init
class ArimaContainer(TimeSeriesContainer):
def __init__():
....
self.return_order_related_params()
def return_order_related_params(self):
"regressor__fit_intercept": [True, False], | ||
"regressor__normalize": [True, False], | ||
} | ||
tune_distributions = { |
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.
Same here, it is not easy to follow the __init__
method, maybe for all classes could go methods like get_tune_grid
and get_tune_distributions
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.
maybe these methods get_tune_grid
and get_tune_distributions
can be a part of the parent class TimeSeriesExperiment
, we can actually force each container to have this method trough an abstractmethod
.
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 like the idea. Currently, I was just following the approach that already existed in other modules, but this modularization is definitely something that I would like to have as well. Since it can potentially be leveraged by other tasks as well (regression, classification, etc.), lets quickly discuss with @Yard1 before making this change. I will open a Issue for this so we keep track of it,
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.
great, thanks Nikhil.
Describe the changes you've made
Added more scikit models
Type of change
How Has This Been Tested?
Unit tests are passing locally
Describe if there is any unusual behaviour of your code(Write
NA
if there isn't)None
Checklist: