So, it would appear we have a problem with grid search and descendants. FZI, @aiwalter, @GuzalBulatova, @mloning, @TonyBagnall.

The problem is not in the refactoring, but that the grid search even before refactor seems to have implemented polymorphic behaviour regarding the "requires fh in fit" property. That is, it was behaving as if that tag was dynamically set (by delegation).

Since the refactor makes this more explicit by defaulting to base logic, this no longer works, and, depending on the setting of the fh tag, either breaks in testing when the inner forecaster can´t deal with fh in fit, or in many places (including examples) because it starts to require fh in fit per the tag.

I see three feasible solutions here:

1. introduce dynamic tags, not generally but just localized to grid search, to mimick pre-refactor behaviour. This requires modification to _all_tags but seems the cleanest solution to me. @mloning may find this controversial though.
2. set the tag to the "safe" version, i.e., requires fh in fit; change all instances of calls to grid search fit to have fh. This would mean re-writing the battery of tests involving grid search, as well as the forecasting tutorial.
3. do nothing, i.e., revert grid search to pre-refactor state, add back the mixins etc, accept that we never finish the refactor

Any opinions or other options, @aiwalter, @GuzalBulatova, @mloning , @TonyBagnall?

Hi @fkiraly, I looked at the failing test when setting "requires-fh-in-fit": False (what the static/class-level tag should be). To me, it seems to be primarily a dynamic attribute delegation issue rather than a tag issue. We're delegating the methods to the wrapped forecaster, but not the fh, cutoff and _y attributes. There are some complications, as we cannot always delegate to best_forecaster_ because it is only available after fitting, whereas we expect some of these attributes already before fitting. Does that make sense?

I have implemented that in PR #1092, let me know what you think?

Does that make sense?

I would prefer a solution where we do not delegate the fit/predict level attributes to the best forecaster, i.e., fh and cutoff etc, since this makes the delegation fiddly and brittle - also, it would step the grid search out from any changes made to the base class due to the overrides here.

I prefer a solution where changes to the base class on the top level fit/predict directly inherit through to the grid search properly, instead always needing 1:1 maintenance when the base class changes.

In addition, I think that it makes sense for the wrapping and the wrapped estimators to keep track of fh and cutoff etc separately, for interface hygiene.

There are some complications, as we cannot always delegate to best_forecaster_ because it is only available after fitting, whereas we expect some of these attributes already before fitting.

These complications are what leads to unnecessary overrides, in my opinion.

What makes much more sense, i.m.o., is to delegate _fit and _update etc, as I've done in #1091, and as was done in @GuzalBulatova's first drafts.

I've skimmed through the discussions and unfortunately I do not know/understand enough about BaseGridSearch to make an informed decision.

After a first glance at fk's PR 1091, I lean towards it being 'over-engineered' and it is tricky to work out what exactly what is going on. The base class is suddenly filled with a lot of extra baggage. (note, I have only looked at base class so far...I'm not exaggerating when I say I can't make informed decision!).

Markus' PR is nice at first glance becaue the changes are minimal and seem easier to understand. But Franz has pointed out some flaws in his comment above (which I do not fully understand).

My instinct is that the 'best' way of doings things will be one which only changes BaseGridSearch without changing _base.py; whether that is Markus' solution or something else, I have no idea.

My other instinct is that we should go with FK's solution only if we anticipate using the extra machinery elsewhere (maybe in #1079 ??)

We can delegate to the internal methods, this avoid dynamic attribute delegation. I changed that in #1092 and it seems to work. Note that this

• assumes that all forecasters passed to grid search CV follow the same internal API as in sktime
• now raises a NotImplementedError rather than an AttributeError if the delegate inherits from BaseForecaster but does not implement the internal method,
• uses different ways of delegating different methods (e.g. update vs transform),
• tests pass only because the forecaster passed to grid search CV in testing (NaiveForecaster) does not require the fh in fitting.

So it's also not perfect, but none are major points for me at this stage.

I largely agree with @Lovkush-A's comments above. If we can avoid making this PR conditional on dynamic tags, I would strongly prefer so, as we haven't given that much thought (see #981).

I think it's fine to use either option (attribute delegation or delegation to internal methods) for now, merge this, finish the first round of refactoring, make a release and then revisit the broader topics such as dynamic tags.

We should also move some of the tests from sktime/forecasting/tests/test_all_sktime_forecasters.py to sktime/forecasting/tests/test_all_forecasters.py - happy to do this in #1092.

tests pass only because the forecaster passed to grid search CV in testing (NaiveForecaster) does not require the fh in fitting.

This is actually my major concern!

The dediated interface point for "requires the fh in fitting" diverges from its intended and explicitly stated meaning.

So that the test passes is a consequence of the chosen default settings in testing, rather than actual correctness of the interface!
As you said, if you would use other default values (a component that requires fh in fitting), the same test would break; worse, it looks like you can't satisfy both variants of the test with the #1092 strategy.

I.e., there's both a problem with the test currently, as well as the proposed solution - but the test doesn't catch it since it has a problem (see above).

I would strongly opine that my proposal is a principled way to fix this "broken interface" issue - and, currently, the only way that is on the table as an implemented solution.

I'm happy to defer this until after the next release, but what I don't want is that we just indefinitely block the issue out by stalling discussion of it, and ultimately forget about it and never resolve it until something breaks and we have no idea why.

Okay, let's defer this until after the next release. Can I merge my suggestions then in #1092?

I'm happy to defer this until after the next release, but what I don't want is that we just indefinitely block the issue out by stalling discussion of it, and ultimately forget about it and never resolve it until something breaks and we have no idea why.

I don't think any of us wants this.

Can I merge my suggestions then in #1092?

As suggested in #1092, I would be happy with the double merge if at the same time we wrap up BaseObject in the proposed way.

@mloning, no, let's get done with this :-)

actually...

here's a question: are major version steps based on breaking usage contracts, extension contracts, or internal contracts?

Because the removal of the mixins and _SktimeForecaster breaks an extension contract (any external forecaster implementation as per implicit guidelines), but not the usage contracts (usage behaviour of sktime is unchanged since downwards compatible).

I think it's fine to go in, I'm not too worried about internal contracts, especially since there wasn't any documentation on it. Probably changes now that we have the extension templates. So feel free to merge once all tests pass.

@fkiraly if you approve, you can merge

Ah realised that you can't approve, so I merged now - that finishes the refactoring I believe, great work, the code looks a lot better now!