Change default high cardinality encoder to StringEncoder

[rcap107]

This is a draft PR for changing the default encoders from Gap (in the TableVectorizer) and Minhash (in the tabular_learner) to StringEncoder in both cases.

[rcap107]

Two tests are failing and I am not sure what's the best way to address the issues

When testing the tabular_learner, test_linear_learner is expecting a GapEncoder as default parameter, but the default parameter is now "warn", so the assert fails

I tried to address the TableVectorizer test by adding a filterwarning, but I managed to turn that into an internal error of pytest by doing that

[jeromedockes]

@glemaitre talking with @rcap107 we were thinking the simplest thing here would be to warn in __init__ if the given high_cardinality is HIGH_CARDINALITY_TRANSFORMER. But I guess in scikit-theory we should not warn in init but in fit. however that means having a less informative attribute like "warn" . WDYT?

[glemaitre]

I have a high bias towards what scikit-learn do. However, I'm not sure that our encoders are passing the test suite so maybe we could avoid it.

[glemaitre]

Also I see that you are using a DeprecationWarning. In scikit-learn, we would use a FutureWarning instead. DeprecationWarning are usually silenced by default and more intended to developers.

[jeromedockes]

what scikit-learn do.

in this case (change the default value of an argument) what is the "canonical" approach?

[glemaitre]

Indeed, we use the "warn" string in __init__ and check in fit.

We document it here: https://scikit-learn.org/dev/developers/contributing.html#change-the-default-value-of-a-parameter

[jeromedockes]

Indeed, we use the "warn" string in init and check in fit.

ok but then the attribute is "warn" so we cannot set params on the subestimator if we use the default right. which is why the current defaults are what they are and we clone in the init -- I don't know if you remember that discussion from way back

[GaelVaroquaux]

I'm fine being a bit more brutal here than in scikit-learn with backwards compatibility.

…

On Apr 30, 2025, 17:23, at 17:23, "Jérôme Dockès" ***@***.***> wrote: jeromedockes left a comment (skrub-data/skrub#1354) > Indeed, we use the "warn" string in __init__ and check in fit. ok but then the attribute is "warn" so we cannot set params on the subestimator if we use the default right. which is why the current defaults are what they are and we clone in the init -- I don't know if you remember that discussion from way back -- Reply to this email directly or view it on GitHub: #1354 (comment) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***>

[glemaitre]

Yep. I'm fine to be pragmatic and have something working.

[rcap107]

In this case, which one is the "pragmatic approach"?

[jeromedockes]

In this case, which one is the "pragmatic approach"?

I think issuing the warning in _clone_if_default and keeping everything else the same

[rcap107]

should the deprecation be more evident in the changelog?

[jeromedockes]

I guess we can remove this?

[jeromedockes]

should the deprecation be more evident in the changelog?

I think so, maybe by putting it at the top of the changes section and maybe even using a

.. warning::

   high cardinality changed...

directive?

also in the docstring itself, at the end of the paragraph about this encoder, you can use a directive similar to:

        .. versionchanged:: 0.22
            ``cv`` default value if None changed from 3-fold to 5-fold.

(search for "versionchanged" in the scikit-learn codebase and you will find many examples)

[jeromedockes]

LGTM, thanks a lot @rcap107 ! this will speed-up a lot of pipelines that use the tablevectorizer :)

[Vincent-Maladiere]

LGTM! Thanks @rcap107