[MNT] Hotfix tag scitype:y typo

[aiwalter]

Fixed typo y:scitype, renamed to scitype:y

Reference Issues/PRs

related #1141 #1083 #1376

[fkiraly]

@aiwalter, ouch, this is very painful - no one noticed this?

Why were the tests not failing with the wrong tag name?

[fkiraly]

This makes me think: should get_tag throw an error if it tries to retrieve a non-existing tag, instead of None?

[aiwalter]

I think it can still return None, but only if the tag exists at all. If I give myWrongTag as a tag, it should throw an error in my opinion

[fkiraly]

Yes, agree with the user expectation and default behaviour, @aiwalter.

When I implemented this, I was mimicking the previous default behaviour which was using dict.get, which leads to unexpected results if the tag does not exist (e.g., due to typo), as you say.

Here's my alternative suggestion:
#1450

[fkiraly]

I think else it should raise an error, since this is an unexpected input!

[fkiraly]

@aiwalter, @mloning, I have identified the (or a) source of the failing tests.

The reason is that clone does not clone dynamic tags properly.
This results in some copies having the "univariate" tag value set that the estimator inherits by default, which makes the grid search fail as soon as it encounters a component that is set to "univariate" and not the expected "both" tag value.

There are multiple potential solutions for this:

• ensure that dynamic tags are set in _fit and not in __init__ - this may be confusing to extenders. In this particular case, you'd have to move lines 456-469 in _tune from __init__ to the start of BaseGridSearch._fit.
• write our own, sktime version of clone which adds the line of copying over the _tags_dynamic dict. This is prototyped in PR sktime clean clone method #1454
• introduce a new interface point for implementers, a init_dynamic_tags or similar, which is called at the start of fit.
• introduce a new interface point for intialization of tags and components, init_tags_components, which initializes dynamic tags and component estimators, and is called at the end of the base __init__.

I would be in favour of no.2 and would be against no.1. I would prefer no.4 above no.3.

[fkiraly]

@aiwalter, I found another issue after fixing the above one.

Namely, it comes from evaluate, which is called in grid search but only lets pd.Series through.
However, in the multivariate case it may be called with a pd.DataFrame.

So, it means, currently the grid search algorithms are not capable of dealing with multivariate data, and the tag should always say "univariate".

I've created a PR containing this temporary fix, #1455.

[aiwalter]

I think its contained also in my PR #1376

[fkiraly]

Yes, @aiwalter, but I'd say, let's fix the issues first before we add more stuff.
Proposed merge sequence is #1436, #1454, #1455, #1456, #1449, #1450, also see discussion on slack.
There are three bugs here that we didn't detect since the tests broke.

[aiwalter]

Thx @fkiraly , will look into it tomorrow

[fkiraly]

Hm, looking into #1376, it appears you already discovered two, the issue with the cv splitter and the univariate tag.

Yes, I'd say, let's fix these separately and ensure we have a "clean slate" with tests fixed before we add more content - just to be sure we don't introduce any follow-on bugs.

[fkiraly]

For instance, I'm worried that your #1376 doesn't fix the NaiveForecaster issue of the missing update, but the tests still seem to pass?

[aiwalter]

lets get this merged?

[fkiraly]

Yes, this should fix all the test issues now.

Looks like we got it fixed!
Great, thanks!