minor bugfix - setting _is_fitted to False before input checks in forecasters#941
minor bugfix - setting _is_fitted to False before input checks in forecasters#941
Conversation
|
Could you explain why this is needed for forecasters but not other estimator types? Is this related to the setting of the forecasting horizon? If that's the case, an alternative may be to pass the name of the caller method to the |
Correct. It is to allow
Yes, but it feels less python zen, and less consistent with the |
mloning
left a comment
There was a problem hiding this comment.
I've thought about this a bit more and I prefer the option where we pass the name of the calling method to _set_fh, e.g. _set_fh(fh, method="fit"). For me, the main reasons are:
- it keeps the functionality for checking/setting the
fhcontained to that function and does not assume state changes elsewhere in the code, like at the beginning offit, - it keeps it closer to the implementation of other estimator types where we do not re-set the state at the beginning of
fit, - it avoids the redundancy of having to set
_is_fitted=Falsein the constructor and at the beginning offit.
What do you think, @fkiraly?
Ok, happy to consider this as an endstate. But, practically, going there would be much more more invasive than the current proposed change, since it deviates from the current signature (which you introduced). So, I would strongly recommend first carrying out this minimally invasive change, then the inheritance refactor, and then any well-isolated changes to the setters and checkers. To reiterate my suggestion: let's get with minimally invasive effort to the position where we can just refactor |
mloning
left a comment
There was a problem hiding this comment.
The change in the logic of _set_fh seem to be separate from the interface refactor, no? This PR was still easy to review for me, so adding a logic change to _set_fh could be included. In terms of descendants, we would still only have to change one line. Doing the change here would avoid having intermediate code on main.
But happy to follow your sequence of changes.
That's not entirely true sine |
|
so... @mloning - this needs a second review? Or are we fine to merge? |
|
I think it's fine to merge this as an intermediate solution. |
|
@fkiraly FYI I tend to use "squash and merge" for merging into |
This PR is addressing the forecaster base class proliferation discussed in #510, by simplifying the base class inheritance tree and the streamlining the private method logic. Specifically, I've moved the logic from `_SktimeForecaster` and the forecasting horizin mixins into `BaseForecaster`, and adopted uniformly the principle of having a public method (`fit`, `predict`, etc) with checks/plumbing, dispatching to a "core logic" version (`_fit`, `_predict`, etc) where validated arguments can be assumed. The burden on extenders becomes much lighter, since it is now possible to only focus on the "core logic" implementation, instead of having to keep in mind a myriad of inconsistent and constantly shifting conventions in checks and other plumbing. As side effects, if we get this right, this should make a few things on the roadmap easier: * the extension guidelines #464. Right now, the aforementioned implicit conventions are too many and intricate to write useful extension guidelines, in my opinion. * extending to the multivariate case * input/output checks and the eternal data container discussion, this can go in the plumbing As a proof-of-concept regarding ease of extension, this PR also contains a highly annotated extension template in the `extension_templates` folder. In terms of review, the key file is `forecasting/base/_base`, with corresponding changes (contraction and deletion) in `base/_sktime`. I've tried to keep the interface consistent as much as possible (only changing internal logic). Interface contracts with all the earlier estimators are still honoured, via loopthroughs and default behaviour that ensures that everything still works if `fit`, `predict` etc are overridden by the current descendants, as opposed to `_fit` and `_predict`. The only change I had to made to descendants is set `self._is_fitted=False` at the start of `fit`, which is a minimally invasive change that's also separately reviewable as PR #941.
This is a minuscule change which is a preemptive fix.
The change sets
self._is_fitted = Falsein the first line offit, of a number of forecasters, before the input checks.This prevents some unexpected behaviour when
fit, of the estimator, is called multiple times in sequence, e.g., in grid search (as in the tutorial notebook).It also prevents exceptions caused by this unexpected behaviour in any future refactor (e.g., #912) where
_set_fhuses_is_fittedto check whether anfhis expected.This should be done in all forecasters.