✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 5ed9133. Link to the linter CI: here}

This build failure was solved in scipy/scipy#15998, requires scipy>=1.9.0

OK, so I would probably wait that we bump the minimum version to scipy 1.9 in this case. Can you report the failure and your finding in the original issue?

@xuefeng-xu For the time being, you can make the new code path conditional on scipy>=1.9 with a todo note to remove the old code once our minimum scipy is >= 1.9.

@lorentzenchr @glemaitre I have updated the code, now it's ready for review.

Maybe, there’s a misunderstanding. Could make the call to scipy.yeojohnson conditional on the installes scipy version >= 1.9 and call the old code path otherwise.
Also, please add a test that both versions are equivalent.

@lorentzenchr Do you mean I should keep the _yeo_johnson_transform function?

Meanwhile, can I use this test below, where X_1col is defined here

def test_yeojohnson_for_different_scipy_version():
    pt = PowerTransformer(method="yeo-johnson").fit(X_1col)
    assert_almost_equal(pt.lambdas_[0], 0.99546157)

@lorentzenchr Do you mean I should keep the _yeo_johnson_transform function?

Yes because we still support older scipy versions.

Not sure why the results are different.
The X_1col is different when I manually create it, so the results are different. (This seems weird since I use the same RandomState.)

I tested two scipy versions (1.8.0 and 1.11.1), both of them pass the test case.

Here is a minimal implementation of PowerTransformer, using the scipy.stats methods.
Contrary to the scikit-learn variant , yeojohnson_normmax() manages to find lambda values without overflows and errors.
This snippet assumes that X is a pandas DataFrame.

The parallelization does not scale well with the number of cores.
3 Threads with backend='threading'
circa 8 Threads with backend='loky'

import numpy as np
import pandas as pd
from sklearn.base import BaseEstimator, TransformerMixin, OneToOneFeatureMixin
from joblib import Parallel, delayed
from scipy.stats import yeojohnson_normmax, yeojohnson

class YeoJohnsonTransformer(BaseEstimator, TransformerMixin, OneToOneFeatureMixin):
    def fit(self, X : pd.DataFrame, y=None):
        self._n_rows, self._n_cols = X.shape
        
        jobs = []
        for col_idx in range(self._n_cols):
            col = X.iloc[:,col_idx]
            col = col.to_numpy()[np.isfinite(col)] # drop inf and nan , convert to numpy array
            jobs.append( delayed(yeojohnson_normmax)(col) )

        self._lambdas = Parallel(n_jobs=4, backend='threading', verbose=0)(jobs)
        return self

    
    def transform(self, X : pd.DataFrame):
        X_out = pd.DataFrame(data=np.empty_like(X, dtype=np.float64), index=X.index, columns=X.columns, dtype=np.float64)
        
        for col_idx, lambda_i in zip(range(self._n_cols), self._lambdas):
            col = X.iloc[:,col_idx].to_numpy()
            X_out.iloc[:,col_idx] = yeojohnson(col, lmbda=lambda_i)
        return X_out

Hello,
It seems that this PR has been pending for a while. However it seems almost complete.

Could you confirm that the only remaining tasks are

• to improve the coverage
• to update the changelog

@lesteve ?

In order to improve the coverage, we need to test against scipy 1.9. Which strategy do you suggest to perform this ? Is there a specific CI runner to configure to add scipy 1.9 ?

If needed we ( https://github.com/Projet-open-source - a student project team that I coach ) could give it a try

From a quick look:

• use sklearn/utils/fixes.py to put the function that switches to scipy implementation if scipy is recent enough (look around at other examples in this file for inspiration)
• add a non-regression test and make sure they fix the original issue for example Yeo-Johnson Power Transformer gives Numpy warning (and raises scipy.optimize._optimize.BracketError in some cases) #23319 (comment). According to ENH Use scipy.stats.yeojohnson in PowerTransformer #27818 (comment) it seems the test that was added was not a good non-regression test
• add changelog and mention @xuefeng-xu as co-author. Instructions for adding a changelog are in https://github.com/scikit-learn/scikit-learn/tree/main/doc/whats_new/upcoming_changes#readme.

We already have a CI that tests the minimal scipy version, currently 1.6 according to

Hi @lesteve , a few questions just to make sure I understand correctly.

use sklearn/utils/fixes.py

Do you mean I need to move the code inside the _yeo_johnson_optimize function to the fixes.py file? Doing this also need to add the _neg_log_likelihood and _yeo_johnson_transform functions.

add a non-regression test

The overflow issue was fixed in scipy 1.12 (see scipy/scipy#18852 (comment)). Do I need to add similar test using the overflow data?

@xuefeng-xu sorry I should have been more explicit, my comment was for @smarie. For clarity's sake, @smarie contacted me because he has a small project working with engineering school students to introduce them to open-source contribution.

I came across this PR when I was doing some triage a few weeks ago through #30281 and thought this would be a good PR to finish. My assumption was that as many other scikit-learn PRs, too much time had passed since it was opened for the author (in this case you) to be still interested in contributing ...

As I mentioned my plan was to mention you in the changelog and add you as coauthor when the new PR is merged. Hopefully you find this acceptable 🙏.

Thanks @lesteve ! To be very clear : @xuefeng-xu if you have enough time to perform all items that are mentioned above in the upcoming weeks, please do so - there are plenty of other issues to contribute to in scikit-learn so we will find something else :).

However if you do not have the bandwidth in the upcoming weeks, as suggested by @lesteve we will complete the work on your behalf and of course add you as coauthor.

Let us know !

Thank you @lesteve @smarie ! Please feel free to open a new PR — it's wonderful to hear about the opportunity for students to get involved in open-source contributions.

Who is now working on this issue in which PR? I would really like to get it in.

@lorentzenchr it looks like there was some activity last week in Projet-open-source#3.

My understanding is that @smarie is first working with students in a fork PR and that they will open a scikit-learn PR when it is closer to completion 😉. Once they open the scikit-learn PR, I guess they should probably ping you and me, because we are both interested by helping to get it in 🤞.

Indeed @lesteve we're almost there :) but since this is a student project we did a first pass outside of this repo so as not to pollute too much. You should hear from us very shortly