scikit-learn 1.6 changed behavior of growing trees #30554

sebp · 2024-12-28T22:31:43Z

Describe the bug

While porting scikit-survival to support scikit-learn 1.6, I noticed that one test failed due to trees in a random forest having a different structure (see this GitHub Actions log).

Using git bisect, I could determine that #29458 is the culprit.

The PR imports log from libc.math:

scikit-learn/sklearn/tree/_partitioner.pyx

Line 14 in 23c1965

from libc.math cimport isnan, log

Previously, logwas imported from ._utils:

scikit-learn/sklearn/tree/_splitter.pyx

Line 10 in 215be2e

from ._utils cimport log

which actually implements log2:

scikit-learn/sklearn/tree/_utils.pyx

Lines 65 to 66 in 215be2e

    
           cdef inline float64_t log(float64_t x) noexcept nogil: 
        
               return ln(x) / ln(2.0)

Replacing

 from libc.math cimport isnan, log

with

from libc.math cimport isnan, log2 as log

fixes the problem.

Steps/Code to Reproduce

from collections import namedtuple
import numpy as np
from sksurv.datasets import load_whas500
from sksurv.column import standardize, categorical_to_numeric
from sksurv.tree import SurvivalTree

from sklearn.tree import export_graphviz

DataSetWithNames = namedtuple("DataSetWithNames", ["x", "y", "names", "x_data_frame"])


def _make_whas500(with_mean=True, with_std=True, to_numeric=False):
    x, y = load_whas500()
    if with_mean:
        x = standardize(x, with_std=with_std)
    if to_numeric:
        x = categorical_to_numeric(x)
    names = ["(Intercept)"] + x.columns.tolist()
    return DataSetWithNames(x=x.values, y=y, names=names, x_data_frame=x)


whas500 = _make_whas500(to_numeric=True)

rng = np.random.RandomState(42)
mask = rng.binomial(n=1, p=0.15, size=whas500.x.shape)
mask = mask.astype(bool)
X = whas500.x.copy()
X[mask] = np.nan

X_train = X[:400]
y_train = whas500.y[:400]
weights = np.array([
    4,5,1,1,2,1,1,1,2,1,0,1,0,1,4,2,0,0,1,0,1,0,1,2,1,1,1,1,1,0,0,0,1,1,3,1,2,1,2,1,0,3,1,0,0,3,0,1,4,1,0,0,2,1,0,1,0,
    1,0,2,1,0,1,1,4,4,2,1,2,2,4,2,1,1,2,1,0,1,0,1,0,0,1,1,1,1,1,1,1,1,1,0,1,3,0,3,0,1,1,1,3,0,1,2,2,3,0,0,1,1,2,0,0,2,
    0,0,1,0,0,1,2,1,2,0,1,1,0,0,0,2,1,1,2,1,0,1,0,1,1,0,2,0,0,1,0,0,1,0,0,0,1,0,0,0,0,0,1,2,0,0,0,0,0,0,0,2,3,0,0,2,0,
    0,1,2,0,1,0,1,2,0,0,0,0,1,2,0,0,2,0,0,0,1,0,2,2,0,1,0,1,4,1,0,0,3,0,1,1,0,1,1,0,2,0,2,1,4,0,1,0,1,0,2,1,3,1,0,0,2,
    1,1,0,2,1,2,2,0,2,0,0,1,1,1,3,0,2,2,0,0,1,3,0,2,0,0,1,1,4,1,0,0,1,1,2,1,1,1,2,0,2,1,1,1,2,0,0,0,1,0,0,2,0,0,0,0,0,
    0,3,1,2,0,3,1,4,1,2,0,0,1,1,2,2,1,1,3,1,1,1,1,1,0,0,0,0,2,2,1,2,0,2,1,2,0,2,0,1,0,0,1,1,1,1,1,3,1,2,0,2,2,2,1,3,1,
    0,0,0,0,0,1,0,2,2,1,1,2,0,0,0,2,2,1,0,1,0,2,0,1,0,2,2,0,3,2,2,1,0,3,0,0,2,2,0,1,0,2,1,1,0,0,2,1,1,0,0,2,1,0,0,2,2,3
], dtype=float)

t = SurvivalTree(
    low_memory=True,
    max_depth=3,
    max_features='sqrt',
    max_leaf_nodes=None,
    min_samples_leaf=3,
    min_samples_split=6,
    min_weight_fraction_leaf=0.0,
    random_state=1608637542,
    splitter='best',
)
t.fit(X_train, y_train, weights)

export_graphviz(
    t, "tree.dot", label="none", impurity=False
)

Expected Results

Actual Results

Versions

System:
    python: 3.13.0 (main, Oct  7 2024, 23:47:22) [Clang 18.1.8 ]
executable: /…/.venv/bin/python
   machine: macOS-15.2-arm64-arm-64bit-Mach-O

Python dependencies:
      sklearn: 1.6.0
          pip: None
   setuptools: 75.6.0
        numpy: 2.2.1
        scipy: 1.14.1
       Cython: 3.0.11
       pandas: 2.2.3
   matplotlib: None
       joblib: 1.4.2
threadpoolctl: 3.5.0

Built with OpenMP: True

threadpoolctl info:
       user_api: openmp
   internal_api: openmp
    num_threads: 8
         prefix: libomp
       filepath: /opt/homebrew/Cellar/libomp/19.1.6/lib/libomp.dylib
        version: None

The text was updated successfully, but these errors were encountered:

thomasjpfan · 2024-12-30T05:01:14Z

Thank you for the report! I agree this is a bug & opened #30557 to fix it.

lesteve · 2025-01-04T12:14:19Z

@sebp thanks a lot for the reproducer and the diagnostic! As a maintainer, high quality reports like yours are so useful 🙏!

Something I would suggest if you think this makes sense for your project: test scikit-learn development wheels so you can give feed-back closer to the time the PR is merged rather than after we release.

If you think it it not worth it for your project, it is completely fine of course! For example, if this is the first time in 5 years that a new scikit-learn release breaks scikit-survival, then this is probably not worth it ...

If you think this is worth it, I would be keen to help a bit on the CI set-up side, so let me know! This would be an opportunity for me to realise how much work this is and get a better feeling of the trade-off involved.

The basic idea is to install scikit-learn development wheel (built daily from main) through a command like this:

pip install --pre --extra-index https://pypi.anaconda.org/scientific-python-nightly-wheels/simple scikit-learn

see https://scikit-learn.org/dev/developers/advanced_installation.html#installing-nightly-builds. There is a number of other projects, in case you feel more adventurous and want to test other projects dependencies (numpy, scipy, matplotlib, etc ...) see https://anaconda.org/scientific-python-nightly-wheels/repo for the full list.

Why you would want to do this? (optional reading 😅)

We have been doing something similar in scikit-learn for numpy and scipy for some time, and I find this extremely useful for all the projects involved.

Advantages for downstream (scikit-learn in our case, scikit-survival in your case)

this does not break too often so not such a pain to maintain for downstream (scikit-learn in our case, scikit-survival in your case) after the initial CI set-up cost.
figuring/guessing which PR may be the culprit is easier since you generally have only a few commits between two nightly wheels

Advantages for upstream (numpy or scipy in our case, scikit-learn in your case)

getting feed-back about unwanted side-effects. It is easy to miss edge cases or have blind spots in our tests.
the feed-back for upstream gets is quite close to when the PR was merged, so PR contributors still have the details of the PR fresh in their mind and the fix tends to be less painful. Contrast this to having the feed-back a few months after you work on something ...

Advantages for the longer-term health of the ecosystem

we learn about upstream and downstream projects and understand each other a bit more
this strengthens links and build trust between the projects
we make the ecosystem more robust

Feel-good stories about this kind of cross-projects interactions (optional reading 😅)

Here are a few of these interactions I managed to find (mostly because they happened not so long ago) and that are part of my personal feel-good open-source stories:

weird timeout noticed by MNE-Python on Windows -> report to scipy -> ended up being fixed in OpenBLAS. Personally, I was waayyyyy outside of my comfort zone but with inputs and work from other scikit-learn, scipy and OpenBLAS developers, eventually the issue was fixed 🎉. I have no idea how the OpenBLAS fix could actually fix anything but you know as long as it is fixed 🤷 😅.
Failure with scipy release candidate reported by scikit-lego -> report in scipy. I ended up proposing a fix that was accepted (I felt too bad about asking scipy to revert close to the release). Full disclosure: I think this was was noticed by manual release candidate testing rather than testing development wheels.
numpy-dev broke our tests, automatic issue opened, I commented in numpy PR asking for inputs and that was fixed in this numpy PR. Similar thing as previously, I am outside of my comfort zone but with my input, numpy developers were able to figure out what the problem was and how to fix it.

Due to scikit-learn/scikit-learn#30554

sebp · 2025-03-15T21:02:15Z

Thanks for your feedback. Testing against nightly builds of scikit-learn could actually be quite useful, because typically each new release of scikit-learn requires some changes applied to scikit-survival too.

sebp added Bug Needs Triage Issue requires triage labels Dec 28, 2024

sebp mentioned this issue Dec 28, 2024

Add support for scikit-learn 1.6 sebp/scikit-survival#504

Merged

4 tasks

thomasjpfan mentioned this issue Dec 30, 2024

FIX Uses log2 in tree building #30557

Merged

thomasjpfan added module:tree and removed Needs Triage Issue requires triage labels Dec 30, 2024

jeremiedbb closed this as completed in #30557 Jan 2, 2025

sebp added a commit to sebp/scikit-survival that referenced this issue Jan 11, 2025

Require scikit-learn 1.6.1

d9e2c2f

Due to scikit-learn/scikit-learn#30554

sebp added a commit to sebp/scikit-survival that referenced this issue Jan 11, 2025

Require scikit-learn 1.6.1

83618d4

Due to scikit-learn/scikit-learn#30554

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scikit-learn 1.6 changed behavior of growing trees #30554

scikit-learn 1.6 changed behavior of growing trees #30554

sebp commented Dec 28, 2024 •

edited

Loading

thomasjpfan commented Dec 30, 2024

lesteve commented Jan 4, 2025 •

edited

Loading

sebp commented Mar 15, 2025 via email

scikit-learn 1.6 changed behavior of growing trees #30554

scikit-learn 1.6 changed behavior of growing trees #30554

Comments

sebp commented Dec 28, 2024 • edited Loading

Describe the bug

Steps/Code to Reproduce

Expected Results

Actual Results

Versions

thomasjpfan commented Dec 30, 2024

lesteve commented Jan 4, 2025 • edited Loading

Why you would want to do this? (optional reading 😅)

Feel-good stories about this kind of cross-projects interactions (optional reading 😅)

sebp commented Mar 15, 2025 via email

sebp commented Dec 28, 2024 •

edited

Loading

lesteve commented Jan 4, 2025 •

edited

Loading