Forecasters re-factoring: BaseGridSearch, ForecastingGridSearchCV, ForecastingRandomizedSearchCV by GuzalBulatova · Pull Request #1034 · sktime/sktime

GuzalBulatova · 2021-06-23T17:44:14Z

Reference Issues/PRs

References #955
We need to slowly re-factor all forecasters to be compliant with the new interface (see PR #912). i.e., no overrides of fit etc and descendant classes only implementing core logic, while the BaseForecaster implements check etc logic.

Currently this is running due to various downwards compatibility workarounds, which should be removed once all forecasters are ready (in "no-override" state)

What does this implement/fix? Explain your changes.

❗ added self.check_is_fitted() to BaseForecaster's update_predict fn
changed fit to private _fit
made sure \forecasting\model_selection\tests\test_tune.py pass
added _tags: used the (conservative) values from the extension template

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

_check_is_fitted (line 170) - is it necessary?

fkiraly · 2021-06-24T14:46:54Z

I think the test fails because the forecaster does not raise a "not fitted" error when you call update_predict.
Normally, a call self.check_is_fitted() would take care of this.

Now, it would appear this is missing in the base forecaster already. Can you add it there? That should fix this issue.

fkiraly · 2021-06-24T14:47:09Z

Also, please make sure to update from main.

fkiraly · 2021-06-24T21:00:44Z

To be more precise:

I think you should try to add the line
self.check_is_fitted() at the start of BaseForecaster's update_predict - that would be my best guess.

fkiraly · 2021-06-25T17:29:02Z

this looks done - if you think it's done, can you please convert to PR from draft PR and request reviews?
Make sure to update from main first.

GuzalBulatova · 2021-06-25T17:59:57Z

Thank you @fkiraly , I've updated and opened it for review.

fkiraly

@GuzalBulatova, thanks!

I just noticed a problem: please return the public methods that were not moved "one layer deep" to their original state. This would be get_fitted_params, inverse_transform, score, and evaluate_candidates.

GuzalBulatova · 2021-06-25T22:46:24Z

@fkiraly thanks, I've updated the PR. 👍
Should I remove self.check_is_fitted("score") from score method?
In _base.py the method score has commentary "no input checks needed here, they will be performed in predict and loss function".

fkiraly · 2021-06-25T23:47:29Z

Should I remove self.check_is_fitted("score") from score method?

The mentioned functions should remain entirely unchanged, since we did not change the base classes there.

fkiraly · 2021-06-26T17:45:56Z

I think there is still a problem with transform and inverse_transform? These should be as they were before.

GuzalBulatova · 2021-06-26T19:32:50Z

@fkiraly thank you, I un-did the changes and I think it should all be ready now.

GuzalBulatova · 2021-06-26T20:11:55Z

@fkiraly a question about self.check_is_fitted() in cutoff. I took it out, but it's not in base - should we add it to the BaseForecaster or should I return it in BaseGridSearch?

fkiraly · 2021-06-27T15:35:39Z

@fkiraly a question about self.check_is_fitted() in cutoff. I took it out, but it's not in base - should we add it to the BaseForecaster or should I return it in BaseGridSearch?

Hm, looking at this:

it should remain in BaseGridSearch, since it overloads the cutoff with a good reason (mirror the best forecaster)
but it should also be added in the BaseForecaster

I.e., it should be in both places in the end. In the description of the PR, make a note where you changed the BaseForecaster.

GuzalBulatova · 2021-06-28T14:36:53Z

Hi @fkiraly! I need your advice. After adding fit check in BaseForecaster's cutoff a lot of tests failed. (If add it only to the cutoff in BaseGridSearch all the tests ran fine). As I understand it, there are many estimators that don't call fit. Is there an elegant solution to this? I couldn't figure it out. Thank you!

fkiraly · 2021-06-28T15:19:28Z

Hi @fkiraly! I need your advice. After adding fit check in BaseForecaster's cutoff a lot of tests failed. (If add it only to the cutoff in BaseGridSearch all the tests ran fine). As I understand it, there are many estimators that don't call fit. Is there an elegant solution to this? I couldn't figure it out. Thank you!

Hm, looks like a number of forecasters check the cutoff when it's not yet fitted.
So I would suggest, remove it from BaseForecaster again, to complete this refactor, and let's discuss separately whether it makes sense that some forecaster access the field itself in fitting, or whether that should be done differently, e.g., via a temporary variable.

Other opinions, @mloning, @aiwalter? I don't think we need to solve the issue in this PR, so let's finish the refactoring without further changes to BaseForecaster and re-visit later.

sktime/forecasting/model_selection/_tune.py

fkiraly

looks good, with the exception that an _update_predict is called.

Currently, this method does not exist in the template, so it should be reverted to update_predict.
I left it that way due to the issues and discussions around #982.

mloning

I think this makes sense now as it is, let's see if the tests pass.

fkiraly · 2021-06-30T18:14:46Z

wait, why? Why do we need to override almost everything? This doesn't look right.

fkiraly

ok, I've looked at the code.

Indeed it makes sense, since we need to check tags etc in the delegates.

fkiraly · 2021-06-30T23:06:06Z

merged, great! Last refactor item done, thanks @GuzalBulatova!

fkiraly · 2021-07-01T19:19:09Z

FYI, some follow-on work is happening in #1088, #1091, #1092

Lovkush-A · 2021-07-01T19:23:45Z

I just just about to have a look at this, but couldn't find it in the list of PRs. Good stuff for getting this sorted! 😄

fkiraly · 2021-07-01T19:50:14Z

ah, but it's not sorted yet, @Lovkush-A!

Look at #1088, #1091 and #1092.
Seems like @mloning and I disagree on how to proceed.

My fix is in #1091, and @mloning then suggested to do it differently in #1092.

Added tags, deleted unnecessary checks

d5097cd

fkiraly mentioned this pull request Jun 24, 2021

re-factoring existing forecasters to new interface #955

Closed

16 tasks

Restored cutoff

04cee66

Added back check to update_predict

5b865a7

Guzal Bulatova added 3 commits June 25, 2021 13:59

Merge remote-tracking branch 'upstream/main' into gridsearch-re-factor

2a0ed7f

Added check_is_fitted to BaseForecaster's update_predict

fd888be

Fixed typo in check_is_fitted

81d20db

Merge branch 'main' into gridsearch-re-factor

e10f5d0

fkiraly requested changes Jun 25, 2021

View reviewed changes

Returned 4 public methods to their original state

1bdd596

Guzal Bulatova and others added 3 commits June 26, 2021 02:39

Updated score fn

e79b47b

Merge branch 'main' into gridsearch-re-factor

5e6a0d1

fit checks are back in score and get_fitted_params

582f9f0

reverted back transform and inverse_transform

70a3f25

Guzal Bulatova and others added 2 commits June 26, 2021 21:52

Merge branch 'main' into gridsearch-re-factor

05503d9

returned _check_param_grid to _run_search

ebbf8aa

fit check back in cutoff

5621ee7

Guzal Bulatova and others added 2 commits June 28, 2021 11:22

Merge branch 'main' into gridsearch-re-factor

fe360ca

Added fit check to cutoff fn in BaseForecaster

ff6c9d7

fkiraly requested a review from ninamiolane June 28, 2021 16:48

Guzal Bulatova and others added 2 commits June 29, 2021 11:23

Merge branch 'main' into gridsearch-re-factor

66667fd

Removed fit check from BaseForecaster's cutoff fn

b4fee4c

mloning reviewed Jun 29, 2021

View reviewed changes

sktime/forecasting/model_selection/_tune.py Outdated Show resolved Hide resolved

fkiraly reviewed Jun 29, 2021

View reviewed changes

sktime/forecasting/model_selection/_tune.py Outdated Show resolved Hide resolved

fkiraly self-requested a review June 29, 2021 19:27

fkiraly requested changes Jun 29, 2021

View reviewed changes

Guzal Bulatova and others added 4 commits June 30, 2021 14:56

Merge branch 'main' into gridsearch-re-factor

349a210

Reverted update_predict fn back to public

8e2a7d5

reverted all fns to public except for _fit

ed96c59

fit checks back in public methods

1af4bfc

mloning approved these changes Jun 30, 2021

View reviewed changes

fkiraly approved these changes Jun 30, 2021

View reviewed changes

Merge branch 'main' into gridsearch-re-factor

8ed91f7

fkiraly merged commit 2730c98 into sktime:main Jun 30, 2021

Uh oh!

Conversation

GuzalBulatova commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Uh oh!

fkiraly commented Jun 24, 2021

Uh oh!

fkiraly commented Jun 24, 2021

Uh oh!

fkiraly commented Jun 24, 2021

Uh oh!

fkiraly commented Jun 25, 2021

Uh oh!

GuzalBulatova commented Jun 25, 2021

Uh oh!

fkiraly left a comment

Choose a reason for hiding this comment

Uh oh!

GuzalBulatova commented Jun 25, 2021

Uh oh!

fkiraly commented Jun 25, 2021

Uh oh!

fkiraly commented Jun 26, 2021

Uh oh!

GuzalBulatova commented Jun 26, 2021

Uh oh!

GuzalBulatova commented Jun 26, 2021

Uh oh!

fkiraly commented Jun 27, 2021

Uh oh!

GuzalBulatova commented Jun 28, 2021

Uh oh!

fkiraly commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fkiraly left a comment

Choose a reason for hiding this comment

Uh oh!

mloning left a comment

Choose a reason for hiding this comment

Uh oh!

fkiraly commented Jun 30, 2021

Uh oh!

fkiraly left a comment

Choose a reason for hiding this comment

Uh oh!

fkiraly commented Jun 30, 2021

Uh oh!

fkiraly commented Jul 1, 2021

Uh oh!

Lovkush-A commented Jul 1, 2021

Uh oh!

fkiraly commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GuzalBulatova commented Jun 23, 2021 •

edited

Loading

fkiraly commented Jun 28, 2021 •

edited

Loading