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.

Sign up for GitHub

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

Jump to bottom

Added multivariate TransformedTargetForecaster, ForecastingPipeline, BaseGridSearch, MultiplexForecaster #1376

Merged
merged 124 commits into from Oct 23, 2021
Merged

Added multivariate TransformedTargetForecaster, ForecastingPipeline, BaseGridSearch, MultiplexForecaster #1376

merged 124 commits into from Oct 23, 2021

Conversation

aiwalter
Copy link
Contributor

@aiwalter aiwalter commented Sep 2, 2021 •
edited

Reference Issues/PRs

#1364
#220

What does this implement/fix? Explain your changes.

  • Accept isinstance(y, pd.DataFrame) in TransformedTargetForecaster
  • Accept isinstance(y, pd.DataFrame) in BaseGridSearch
  • Accept isinstance(y, pd.DataFrame) in ForecastingPipeline
  • Accept isinstance(y, pd.DataFrame) in MultiplexForecaster

Any other comments?

Waiting for #1301 to be merged, tests will fail before.

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
  • I've added unit tests and made sure they pass locally.
For new estimators
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

thayeylolu and others added 30 commits July 27, 2021 14:40
@thayeylolu
checks sctitype tag type for y multivariate and univariate
8aae00f
@thayeylolu
includes condition for both
fe2833d
@thayeylolu
includes condition for both
553456a
@fkiraly
Merge branch 'main' into fix_test_get_fitted_params
3de4329
@thayeylolu
Merge branch 'main' into fix_test_get_fitted_params
1b302cf
@thayeylolu
include condition for multivariate y
bef1d33
@thayeylolu
fix check to include condition for y multivariate
f2809f4
@thayeylolu
fix check to include condition for y multivariate
9151918
@thayeylolu
renamed y to y_train
da916f0
@thayeylolu
fix check to include condition for y multivariate
37e52be
@thayeylolu
restructures condition logic
febfea7
@thayeylolu
logic for multivariate forecasters
4847562
@thayeylolu
includes condition for multivariate y
ed31c19
@thayeylolu
includes condtion for multivariate
498a7c7
@thayeylolu
includes condition for multivariate y
48c8175
@thayeylolu
includes condition for multivariate y
c23464b
@thayeylolu
includes condition for both y
a8d6143
@thayeylolu
includes condition for both y
e214b7d
@thayeylolu
includes condition for both y
3c3fcad
@thayeylolu
includes condition for both y
879bfc4
@thayeylolu
includes condition for both y
8d4e1d3
@thayeylolu
includes condition for both y
c8140c3
@thayeylolu
includes condition for both y
55f139f
@thayeylolu
includes condition for both y
318bba0
@thayeylolu
Merge branch 'fix_test_predict_time_index_with_X' into fix_test_score
a26758d
@thayeylolu
Merge branch 'fix_test_predict_time_index' into fix_test_score
e46c890
@thayeylolu
Merge branch 'fixupdate__predict_single' into fix_test_score
a8ede9e
@thayeylolu
Merge branch 'fix_test_update_predict_predicted' into fix_test_score
a086d70
@thayeylolu
Merge branch 'fix_predict_pred_interval' into fix_test_score
88c94d2
@thayeylolu
Merge branch 'fix_test_get_fitted_params' into fix_test_score
bb2b360
@aiwalter aiwalter closed this Oct 12, 2021
@aiwalter aiwalter reopened this Oct 12, 2021
mloning
mloning reviewed Oct 15, 2021
View reviewed changes
Copy link
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

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

Hi @aiwalter - looks good! Left a few comments to clarify a few points.

sktime/forecasting/compose/_pipeline.py Outdated Show resolved Hide resolved
sktime/forecasting/compose/_pipeline.py Outdated Show resolved Hide resolved
sktime/forecasting/compose/tests/test_pipeline.py Outdated Show resolved Hide resolved
sktime/forecasting/model_selection/_tune.py Show resolved Hide resolved
sktime/forecasting/naive.py Show resolved Hide resolved
fkiraly
fkiraly requested changes Oct 16, 2021
View reviewed changes
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.

the one point which I would consider a blocker is the change to NaiveForecaster which @mloning mentions, should go in the private version to keep the logic in one place.

sktime/forecasting/tests/test_all_forecasters.py Outdated Show resolved Hide resolved
sktime/forecasting/model_selection/_tune.py Outdated Show resolved Hide resolved
@fkiraly
Copy link
Collaborator

fkiraly commented Oct 17, 2021

@aiwalter, I'll approve if @mloning is happy how you handled _NaiveForecaster.

@aiwalter
Copy link
Contributor Author

Can we get this in @fkiraly @mloning

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 18, 2021

Can we get this in @fkiraly @mloning

I'd like to see @mloning's request resolved, then yes.

@aiwalter
Copy link
Contributor Author

seems Markus is busy these days, if he doesnt accept the solution, I could still do that differently in a separate PR in case he has a better idea.

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 23, 2021

seems Markus is busy these days, if he doesnt accept the solution, I could still do that differently in a separate PR in case he has a better idea.

Fine with me - in my opinion, the open point was addressed, you've moved the in-sample index treatment into _NaiveForecaster.

@fkiraly fkiraly merged commit a2d77aa into sktime:main Oct 23, 2021
@aiwalter aiwalter deleted the multivariate_pipeline branch October 23, 2021 11:26
@mloning
Copy link
Contributor

mloning commented Oct 23, 2021

Yes makes sense, thanks @aiwalter and @fkiraly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thayeylolu thayeylolu Awaiting requested review from thayeylolu

@MatthewMiddlehurst MatthewMiddlehurst Awaiting requested review from MatthewMiddlehurst

@RNKuhns RNKuhns Awaiting requested review from RNKuhns

@TonyBagnall TonyBagnall Awaiting requested review from TonyBagnall

@GuzalBulatova GuzalBulatova Awaiting requested review from GuzalBulatova

@mloning mloning Awaiting requested review from mloning

@fkiraly fkiraly Awaiting requested review from fkiraly fkiraly is a code owner

Assignees
No one assigned
Labels
module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Milestone
v0.9.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aiwalter @fkiraly @mloning @thayeylolu