Run sklearn check_estimators on the hidimstat estimators that are expected to behave like a sklearn estimator

[Remi-Gau]

See https://scikit-learn.org/stable/modules/generated/sklearn.utils.estimator_checks.check_estimator.html

This could help standardize a few things in the API and behavior of the estimator.

I can open a PR to set up the tests for this to get you started if you are interested.

List of estimators

• BasePerturbation
• BasePerturbationCV
• CFI [MAINT] Run sklearn check_estimators on CFI and CFICV #599
• CFICV [MAINT] Run sklearn check_estimators on CFI and CFICV #599
• CluDL
• DesparsifiedLasso [MAINT] improve sklearn compatibility of DesparsifiedLasso #590
• D0CRT
• EnCluDL
• LOCO
• LOCOCV
• PFI
• PFICV
• ModelXKnockoff

[Remi-Gau]

I can also help navigate those different sklearn checks as I have now been playing with them for almost... (check notes)... almost a year...

[lionelkusch]

It's a good idea, but in my opinion, it's a bit early to implement it because there is a need to be still flexible with the API because we need to have some feedback from users or some experience with the API before stiffening it.

[Remi-Gau]

from personal experience: you want to have those checks running earlier rather than later because fixing things later will take A LOT OF TIME.

also the constraints on the API are not crazy strict: for a lot of them there are more of the type: cannot call score() on an unfitted estimator

also also: those checks are made in a way that allow gradual or optional opt-in: you can skip some checks if they don't make sense in your case.
For example most nilearn etimators have to skip the checks that work on numpy arrays only because nilearn estimators work on images.
Or you can skip or xfail some checks and just say "TODO: compliance with this check has not been implemented yet".

[lionelkusch]

I agree but @bthirion doesn't have a long-term vision enough for having a gain in time with this type of check because the API is not well defined before its implementation. The actual way of doing the API is empirical. Moreover, the politic of @bthirion is "if it ain’t broke, don’t fix it" instead of "prevent bugs instead of waiting for it".
Moreover, it's already the second version of the API in less than 6 months. I won't be surprised if there is a third version of it next year. Adding this type of check will increase the difficulty of the modification of the code and the possibility of having 2 APIs in concurrence.

When there is a more robust base for the API, I think that, at this moment, it will be important to add these checks. This will help people to include new methods in the library and create a base of tests for all models.

[Remi-Gau]

okidoki: I will let you be the judge of it. 😉

[bthirion]

Thx for raising this, but indeed I think it would be a bit premature now.

[Remi-Gau]

@jpaillard I have added a list of estimators the checks should be run on: am I missing any?

[jpaillard]

ModelXKnockoff should also be part of the list