BUG: Refactor LinearModel

[Genuster]

Fixes, internal improvements and a subtle API change for LinearModel.

• make LinearModel proper metaestimator, i.e. self.model should first be cloned to model_ and only then fitted
• set proper sklearn tags
• make LinearModel work with OneVsRestClassifier metaestimator
• explicitly validate that the model is either classifier or regressor
• cover missing tests

[larsoner]

Let me know when I should look!

One comment -- I see a commit message make validation compatible with sklearn<1.6... I would say it's not a big priority. I think it's okay in our test_*_compliance functions to require a fairly recent sklearn (more recent than our minimum supported version) because they probably improve and fix compliance tests over time (and even improve/fix their data validation function we use internally). You can do a pytest.importorskip("sklearn", "1.6") inside the test if you want. But if it doesn't require much work to make the _compliance tests work on older sklearn then feel free to keep working on it.

[Genuster]

Thanks @larsoner, it's more complicated than the _compliance tests. LinearModel would be unusable for users with lower sklearn version because of the __sklearn__tags() call for other classes for parameter validation. But if it's ok to expect sklearn 1.6 for the next MNE versions, then I'll absolutely follow your pytest.importorskip("sklearn", "1.6"), because I can't seem to properly fix this whole thing for the older ubuntu tests. So I'm not sure how to handle this actually.

Let me know when I should look!

Apart from this issue, it's pretty much ready for the review! (btw, the spatial filter visualisation PR is also ready😀)

[larsoner]

LinearModel would be unusable for users with lower sklearn version because of the __sklearn__tags() call for other classes for parameter validation

I don't quite follow... I think __sklearn_tags__ was introduced in 1.6 (right?) so any sklearn version before that should just completely ignore / not use the method. So whatever gymnastics you're doing for those shouldn't affect someone running, say, 1.5 or 1.4. (Our min supported for the next MNE release looks like it'll be 1.3 FYI), right?

[Genuster]

So whatever gymnastics you're doing for those

😁 I was calling __sklearn_tags__ in _validate_params method, which in turn is calling super().__sklearn_tags__ and self.model.__sklearn_tags__ and this would cause errors in old sklearns.
But I changed it now to explicit is_regressor, is_classifier, let's see if can I make these work.

[larsoner]

Ahh I see... yeah maybe better to avoid calling __sklearn_tags__ internally if possible

[larsoner]

@Genuster should I look and merge? FYI we don't get notified for commits or anything so it's a good idea to re-ping for review once you've addressed comments!

[larsoner]

... can you push a commit with [circle full] so we can make sure all examples work? You can use for example

git commit --allow-empty -m 'TST: All examples [circle full]'

Once that's done, LGTM and I think we can merge assuming all examples pass!

[Genuster]

@larsoner yes, it's ready! Will ping you next time :)