Implemented adaptive squashing

[dholzmueller]

WIP

[dholzmueller]

Here is an implementation of the robust scaling + smooth clipping from my paper (https://arxiv.org/abs/2407.04491). It could be used as a robust numerical preprocessing methods for neural network based methods.

Some considerations:

• which name do we want to use for the class? Currently it is AdaptiveSquashingTransformer.
• I did not check the rendering of the documentation. Do we need to link the class from somewhere in the documentation?
• If the column passed in transform() has a different name than in fit_transform(), the output column still gets the name of the column passed in fit_transform(). I copied this behavior from StringEncoder, is that good? (See also the todo in the corresponding tests.)

[Vincent-Maladiere]

Hey @dholzmueller thank you for this PR!

If you want to make your class public, you should add an entry in doc/reference/index.rst, maybe in the "cleaning a dataframe" section?

Also, I think having a smallish example that shows when and where to use your class would be great. WDYT?

[dholzmueller]

Also, I think having a smallish example that shows when and where to use your class would be great. WDYT?

Sounds good, where would you put it?

[dholzmueller]

I addressed everything except from the example now.

[dholzmueller]

I removed the test that tested if a column with name None after transformation still had name None, since it failed for polars. I assume this is not relevant? (The test is not necessary anymore to reach 100% coverage.)

[jeromedockes]

a first few comments, I'll add more. thanks this is great!! 🚀

[Vincent-Maladiere]

Sounds good, where would you put it?

@dholzmueller, good question, I'm not sure. What would be the best way to illustrate its usefulness? Do you have a small benchmark or experiment that could fit into an example? Otherwise we might need to be creative

[dholzmueller]

We could try it on a dataset with an MLP and see if it improves something (compared to StandardScaler? or QuantileTransformer?). Don't know what would be the best one to do this.

[Vincent-Maladiere]

In what setup (dataset + model) have you observed a sharp improvement in your experiments? Otherwise, we could try on any skrub dataset with an MLP as you suggest

[dholzmueller]

Updates:

• renamed the class to SquashingScaler
• made it work for dataframes and not only series by using OnEachColumn with the old class
• renamed the args to lower_quantile and upper_quantile
• incorporated minor suggestions
• added an example using the SquashingScaler

[jeromedockes]

Great, thanks a lot @dholzmueller . I just have a small comment on the fitted attributes but LGTM otherwise :)

[jeromedockes]

even though it wouldn't make a difference here normally we would put the transformermixin first because we want methods to be looked up there before looking in baseestimator

Suggested change

	class SquashingScaler(BaseEstimator, TransformerMixin):
	class SquashingScaler(TransformerMixin, BaseEstimator):

[jeromedockes]

maybe we could make the transformer private by renaming it to self._transformer and exposing its learned attributes directly by copying them to the outer object's dict with something like

if isinstance(self._transformer, OnEachColumn):
    self.median_ = [t.median_ for t in self._transformer.transformers_.values()]
    self.scale_ = [t.median_ for t in self._transformer.transformers_.values()]
else:
    self.median_ = self._transformer.median_
    self.scale_ = self._transformer.scale_

[jeromedockes]

(in this case we could add a section like

    Attributes
    ----------
    median_ : float or list of float
        median of the training data

    scale_ : ...

just below the 'parameters' section in the docstring )

[dholzmueller]

@GaelVaroquaux was suggesting to implement SquashingScaler directly instead of with the indirection through SingleColumnSquashingScaler. I'll need to check how easy that is...

[rcap107]

Overall, I think that the code in this PR is not documented well enough, as there are a lot of parts that "do stuff" in a non-transparent way, and that would benefit from being commented more.

Besides being a problem to maintain (I had to try and reverse engineer most of the code and the tests to figure out how it worked), I think that the scaling could have some unexpected results that may surprise users, and that are not documented or explained properly in the docstrings or in the code itself.

[Vincent-Maladiere]

Hey @rcap107 thanks for reviewing. I iterated on the documentation; let me know what you think

[rcap107]

Thanks for adding all the comments @Vincent-Maladiere, now the code is much clearer 👍

[rcap107]

Hey @rcap107 thanks for reviewing. I iterated on the documentation; let me know what you think

It's much better, thanks a lot! I fixed a small typo, but aside from that I think we can merge this

[Vincent-Maladiere]

Great! Waiting for other people to give their feedback

[dholzmueller]

Interface looks good to me! Did you test what happens if you pass in an all-nan column?

[GaelVaroquaux]

LGTM. Very good!!

[GaelVaroquaux]

I'm waiting for @Vincent-Maladiere 's reply to @dholzmueller 's above, and then merge

[Vincent-Maladiere]

Interface looks good to me! Did you test what happens if you pass in an all-nan column?

Good question. We get a column full of nans, since they are left untouched. Is that what you would expect?

Reproducer:

import numpy as np
import skrub
import pandas as pd

skrub.SquashingScaler().fit_transform(
    pd.DataFrame(dict(a=[np.nan, np.nan]))
)

[dholzmueller]

Yes, that is what I would expect. Just wasn't sure if it's going to work with the nanmax/nanmin operations.

[dholzmueller]

Just a thought, having quantile_range as a tuple instead of two separate values might make it harder for people to tune it in some hyperparameter optimization. I think it's probably not an important hyperparameter to tune, though.

[rcap107]

Just a thought, having quantile_range as a tuple instead of two separate values might make it harder for people to tune it in some hyperparameter optimization. I think it's probably not an important hyperparameter to tune, though.

This can be addressed in another PR

[dholzmueller]

This can be addressed in another PR

you mean, another one for the same release? because it's not as convenient to change the parametes after release, right?

[rcap107]

This can be addressed in another PR

you mean, another one for the same release? because it's not as convenient to change the parametes after release, right?

It's not a very big deal, and it should not be a big change, up to you @Vincent-Maladiere

(I'd rather have this PR in the release and open a new one later than risk not having it)

[Vincent-Maladiere]

That's a good point, both sklearn's RobustScaler and MinMaxScaler have a tuple range as a hyperparameter, so I think this is fine.

[dholzmueller]

okay, then let's leave it like it is.

[GaelVaroquaux]

OK, I suggest that we merge, and worry about the NaN thing later: it can be addressed later

[dholzmueller]

I think the NaN thing is not an issue, since Vincent tested it and it worked.

[GaelVaroquaux]

Merged!

Thanks everyone