-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add prophet regressor #1704
Add prophet regressor #1704
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1704 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 247 249 +2
Lines 19598 19740 +142
=========================================
+ Hits 19589 19731 +142
Misses 9 9
Continue to review full report at Codecov.
|
@@ -514,3 +514,38 @@ def save_plot(fig, filepath=None, format='png', interactive=False, return_filepa | |||
|
|||
if return_filepath: | |||
return filepath | |||
|
|||
|
|||
class suppress_stdout_stderr(object): |
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.
Prophet doesn't have a flag for verbosity so we need this context manager to remove their training output:
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 think we should add a link to the sktime original source
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.
@freddyaboulton the sktime source grabbed it from the issue so I think linking the issue is fine here
@@ -9,3 +9,5 @@ matplotlib>=3.3.3 | |||
graphviz>=0.13 | |||
seaborn>=0.11.1 | |||
category_encoders>=2.0.0 | |||
pystan |
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 left this open for now until we know what versions work with evalml. I can also just use latest versions for now.
estimators_to_check = [estimator for estimator in _all_estimators() if estimator not in [StackedEnsembleClassifier, StackedEnsembleRegressor, TimeSeriesBaselineEstimator]] + [test_estimator_needs_fitting_false] | ||
for component_class in estimators_to_check: | ||
if not component_class.needs_fitting: | ||
continue | ||
|
||
component = helper_functions.safe_init_component_with_njobs_1(component_class) | ||
if component.supported_problem_types == [ProblemTypes.TIME_SERIES_REGRESSION]: |
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.
Prophet does not follow sklearn's estimator API (and this is our first estimator that only does time series problem types) and tests in test_components.py
break so I've added some work arounds here for now. Happy to discuss any of these changes.
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.
Nice! I think it's fine to have this code outside a util function since it's only used twice and only in our tests.
component_obj=prophet_regressor, | ||
random_state=random_state) | ||
|
||
def build_prophet_df(self, X, y=None): |
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.
Prophet requires input to be a single dataframe with a ds
column for dates and y
for targets.
@@ -163,6 +163,15 @@ jobs: | |||
conda config --add channels conda-forge | |||
conda activate curr_py | |||
conda install numba -q -y | |||
- run: |
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.
Need this for windows installation. In the PR to add prophet to automl we should also include installation instructions in our docs.
PR should be good to go. Windows py3.6 tests hang on automl tests for some reason though. I'll take a look into 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.
Looks good! Left a few comments, but agree with Freddy that it might be nice to add a 'column' arg to Prophet's init
so we don't have to guess which column to use, esp if there are multiple datetime cols
evalml/pipelines/components/estimators/regressors/prophet_regressor.py
Outdated
Show resolved
Hide resolved
Still stuck on windows py3.6 tests taking 1hr+. Related to this issue(facebook/prophet#122) but it doesn't seem like their suggestions are working. Will keep working on it. |
This reverts commit 82f21f1.
Fixes #1499.