yes, this looks like a good idea. I might try to get the tags done first, though? Will be next week, though :-/

I think this is more-or-less orthogonal to tags.

Though it will simplify your tags implementation for meta-estimators, for instance.

more or less, yes ;) might be there's no actual issues here. You mean by providing ways to instantiate them? Well it will make it easier to test more classes which will mean there's be many more failures lol ;)

So you wouldn't test that all parameters are either specified by default or explicitly defined as required? We could easily miss "max_iter" not having a default value with this. Maybe not as big an issue for us but for third parties?

Did you set parameters at everything that was impacted by the old function? Or how did you determine when to set parameters? Maybe it's easier to merge this first, either way I'm unlikely to have a lot of bandwidth before next week :-/

I tried to do every edit that corresponded to the existing set_checking_parameters. I've deprecated that function now, but left the portable logic in, on the basis that some implementers may have used it.

I'd really like to hear your opinion on the default parameter tests. It seems the tag for tag was confusing to you and @rth. But I feel it's important to make sure that third party estimators don't have weird required parameters.

And I think leaving the function in but deprecating doesn't really cost us much so I'd do it.

And I think leaving the function in but deprecating doesn't really cost us much so I'd do it.

Done. See https://github.com/scikit-learn/scikit-learn/pull/11324/files#diff-a95fe0e40350c536a5e303e87ac979c4R323

I think you'll like the last commit.

The point is that this will also raise an error if they try run check_estimator on the estimator unless they define params. I think this will help us clearly review any testing params required for contrib projects.

Yeah but it requires a review, and check_estimator has wider use than contrib projects. It leaves open the possibility for someone accidentally forgetting so specify a default for a parameter that they set in testing. But maybe I'm being paranoid.

It could also be that if someone doesn't know our conventions they'll just add them to the testing function and not to the constructor. But maybe the docs can clarify that.

Ok that seems a good solution ;) you pre-empted my complaints.
Needs dev docs, otherwise looks good (though I didn't go through for all the cases).
Let's to his before tags then.

The trouble with that is supporting check_no_attributes_set_in_init. Also, this PR should aim to replace all of check_parameters_default_constructible

The easiest way I can think of doing check_no_attributes_set_in_init is to make _get_test_instances return a dict of params, rather than an instance, and the construction to happen in test_common/check_estimator.

Hm there is tests that test init stuff that are not covered in clone, I think.
I need to double check.

Also from a use perspective is seems easier to pass an instance than add to this function in the top esitmator. Would we want all possibly pipelines we want to test be hardcoded in the pipeline class? That seems weird to me, in particular if I'm testing things like NaN handling.

It would be nice for a third party to be able to check whether their pipeline construct works nicely without inheriting / monkey-patching pipeline.

Maybe I'm sold on the requires_params tag. I tried changing check_parameters_default_constructible to accept some params and make sure that either these or the defaults are set.... but maybe that's silly.

I'm not adamant about my solution at all, will think about your idea later (have to run).
I thought your error message was good but if it's hard to implement thats another thing. I think it might be reasonable to think that people don't start with implementing _generate_test_params so the test will be meaningful in most cases.

Currently the distinction between class level tests and instance level tests is pretty ugly. I was not happy with that at my last refactoring. But I didn't see a good way to check default_constructible but also pass in complex meta-estimators that might not "belong" to a specific class.

Do we want to have this in the release? I was kinda hoping for estimator tags, not sure if I have time to finish it, though...

It would be nice to have all these things... I'm not sure we can justify a delay to release, though!

@jnothman do you mind if I try solving the merge conflicts so we can move this forward?

@NicolasHug I'm not sure there's consensus on this.

I think I'd rather to the meta-estimator initialization by using instance-based testing as in #9741 instead of class-based testing.

The main downside of that is that some estimators (like SelectKBest and GaussianRandomProjection) don't pass the common tests with their default parameters, while other estimators take "a long time" with their default parameters.

Having a decentralized architecture is better than the centralized architecture we have right now, but in the end I feel having "testing parameters" is a bit like playing vw because we're not actually testing the defaults.
I'm not sure if it is worth changing the defaults so they work, but that also seems a bit arbitrary. Or change the tests so they work?

Maybe we need a slep / discussion on if/how to remove set_checking_parameters.

I think I'd rather to the meta-estimator initialization by using instance-based testing as in #9741 instead of class-based testing.

@amueller Could you elaborate on that? Say if I want to run common tests on an estimator with different solvers (#14063), I see how it can be done with this PR but not with instance based approaches (short of manually creating appropriate tests).

this is more or less equivalent to just
calling check_estimator on a bunch of instances of the estimator, with
different solvers for example, and doing so in the estimator's test module.

I agree that would be the most straightforward approach. Is there a reason we haven't been doing that so far? There shouldn't be issues for non meta-estimator I think?

@rth I think the reason we haven't done it is that the last time I was working on this the estimator tags were not merge and so it was not easily possible.

Doing instance-level checks is my preferred solution to checking different code paths. However, it doesn't get rid of set_checking_parameters. I think it's still good to run everything through the common tests so we don't for get anything. But the problem is that some estimators are slow by default and some estimators don't pass the tests by default. Neither of these issues is solved by explicit instance-based testing.

That would still allow a lot of hackery, though. I feel like ideally we shouldn't need to tweak the estimators to pass the tests.

That would still allow a lot of hackery, though. I feel like ideally we shouldn't need to tweak the estimators to pass the tests.

One way to fix the issue is to have more "auto" values for the params. The estimators would ideally set them properly when a small dataset is passed.

@adrinjalali that's only sort-of true. For example if we're checking API we don't need algorithms to convert, so we set max_iter or n_iter to something small.

@jnothman I wasn't necessarily taking about tags. My issue that the point of the tests is to make sure the estimators behave correctly. Any mechanism that allows us to set max_iter to make the algorithm run faster also allows us to hide issues with the default parameters or work around bugs, i.e. cheat the tests.
From that perspective, this PR basically implements the volkswagen method.

And the fast runs use the method in this PR?

In principle that seems like a reasonable solution. Running with default params on the cron job and with "fast" params on PRs?

That would not resolve the issues with weird defaults (like SelectKBest) but we that part could be resolved by being "smart" with "auto"?

Does that mean check_estimator gets a use_testing_parameters argument that propagates to everything, together with the strict_mode parameter?