FIX make sure to reinitialize criterion even when n_missing=0 #28295

glemaitre · 2024-01-27T23:34:25Z

The criterion was not initialize at each split and could wrongly contain information about a previous split containing missing values. I lead to wrong statistic reported by the criterion.

TODO:

include non-regression test as mentioned in the issue
look at the nan value in the regression in the example here: FIX make sure to reinitialize criterion even when n_missing=0 #28295 (comment)

github-actions · 2024-01-27T23:35:36Z

✔️ Linting Passed

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

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

glemaitre · 2024-01-27T23:44:54Z

Not enough. Still have a bug with the 4th estimator in the following forest:

import numpy as np
import sklearn
from sklearn.datasets import load_iris
from sklearn.feature_selection import RFECV
from sklearn.ensemble import RandomForestClassifier
from sklearn.model_selection import train_test_split

X, y = load_iris(as_frame=True, return_X_y=True)

rng = np.random.RandomState(42)
X_missing = X.copy()
mask = rng.binomial(n=np.array([1, 1, 1, 1]).reshape(1, -1),
                    p=(X['petal length (cm)'] / 8).values.reshape(-1, 1)).astype(bool)
X_missing[mask] = np.NaN

X_train, X_test, y_train, y_test = train_test_split(X_missing, y, random_state=13)
clf = RandomForestClassifier(n_estimators=4, random_state=1).fit(X_train, y_train)

The criterion is still nan for node #5. Need to debug it.

glemaitre · 2024-01-28T11:23:53Z

I'm more confident now that we solve the original issue. The integration case on Ames housing show that we are as good as in the imputation case and I have 2 toy examples that I hand crafted that allow me to debug the code and find the issue with some wrong computation of the impurity.

I still have the issue about the np.inf when building the tree but I would tackle this in another issue to understand if it is actually a bug or not.

glemaitre · 2024-01-28T11:24:50Z

@thomasjpfan If you would have a bit of time, it would be great if you can give a look. At the end it is only a 2-lines diff :).

sklearn/tree/tests/test_tree.py

glemaitre · 2024-01-28T15:50:29Z

sklearn/ensemble/tests/test_forest.py

@@ -1854,3 +1860,70 @@ def test_non_supported_criterion_raises_error_with_missing_values():
    msg = "RandomForestRegressor does not accept missing values"
    with pytest.raises(ValueError, match=msg):
        forest.fit(X, y)
+
+
+@skip_if_no_network


This require to fetch Ames housing. I don't know if we could kind of cache the dataset to make sure that we run the test instead of skipping it.

I would very much rather have a toy dataset made for this test rather than downloading one.

Although this test is useful, I prefer to hold off on adding it because running fetch_openml can lead to rare conditions when running with pytest-xdist. There is no race condition now because fetch_openml(("house_prices"), but I rather not establish the pattern.

Note, much of

scikit-learn/sklearn/conftest.py

Line 66 in cb836be

dataset_fetchers = {

was written to "download all the datasets before pytest runs".

For this specific PR, I think test_regression_tree_missing_values_toy is sufficient as a non-regression test.

adam2392 · 2024-01-29T02:18:10Z

Chiming in here w/ some additional context:

I saw some issues when implementing the missing-value support for ExtraTrees in #27966 and applied this fix and also saw empirical performance go up for the ExtraTreeRegressor (one of the unit-tests was having a hard time passing, and I could not figure out why). So this fix definitely is needed :)

There shouldn't be any noticeable performance degradation either since init_missing exits early when n_missing == 0.

adrinjalali

Otherwise LGTM.

adrinjalali · 2024-01-29T10:23:46Z

sklearn/ensemble/tests/test_forest.py

@@ -1854,3 +1860,70 @@ def test_non_supported_criterion_raises_error_with_missing_values():
    msg = "RandomForestRegressor does not accept missing values"
    with pytest.raises(ValueError, match=msg):
        forest.fit(X, y)
+
+
+@skip_if_no_network


I would very much rather have a toy dataset made for this test rather than downloading one.

thomasjpfan

Thank you for the fix! This PR looks right to me.

thomasjpfan · 2024-01-29T18:01:44Z

sklearn/ensemble/tests/test_forest.py

@@ -1854,3 +1860,70 @@ def test_non_supported_criterion_raises_error_with_missing_values():
    msg = "RandomForestRegressor does not accept missing values"
    with pytest.raises(ValueError, match=msg):
        forest.fit(X, y)
+
+
+@skip_if_no_network


Although this test is useful, I prefer to hold off on adding it because running fetch_openml can lead to rare conditions when running with pytest-xdist. There is no race condition now because fetch_openml(("house_prices"), but I rather not establish the pattern.

Note, much of

scikit-learn/sklearn/conftest.py

Line 66 in cb836be

dataset_fetchers = {

was written to "download all the datasets before pytest runs".

For this specific PR, I think test_regression_tree_missing_values_toy is sufficient as a non-regression test.

glemaitre · 2024-01-29T18:53:24Z

I would very much rather have a toy dataset made for this test rather than downloading one.

We actually have 2 datasets in the two other toy tests. Here, this is more an integration test. Actually, it should be given in an example to show the native support for the missing values.

glemaitre · 2024-01-30T08:57:55Z

I remove the integration test. I want to keep this PR focus on the bug fix. Once merge, I propose to have a go into the different examples to remove the SimpleImputer and change narrative and see if I can add back this new example.

…-learn#28295)

glemaitre added 2 commits January 28, 2024 00:29

FIX make sure to reinitialize criterion even when n_missing=0

1c1e34d

ad changelog

0b6be0d

glemaitre changed the title ~~Is/28254~~ FIX make sure to reinitialize criterion even when n_missing=0 Jan 27, 2024

github-actions bot added module:tree cython labels Jan 27, 2024

glemaitre marked this pull request as draft January 27, 2024 23:43

glemaitre added 4 commits January 28, 2024 11:14

iter

ec40194

TST add non-regresion test for regression tree

10e9639

more tests

94893a9

update changelog

1af5f0e

glemaitre marked this pull request as ready for review January 28, 2024 11:21

glemaitre commented Jan 28, 2024

View reviewed changes

sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved

Update sklearn/tree/tests/test_tree.py

ababe98

glemaitre commented Jan 28, 2024

View reviewed changes

adrinjalali reviewed Jan 29, 2024

View reviewed changes

thomasjpfan reviewed Jan 29, 2024

View reviewed changes

remove one test

ff3700d

adrinjalali approved these changes Jan 30, 2024

View reviewed changes

thomasjpfan approved these changes Jan 30, 2024

View reviewed changes

thomasjpfan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Jan 30, 2024

thomasjpfan added this to the 1.4.1 milestone Jan 30, 2024

thomasjpfan enabled auto-merge (squash) January 30, 2024 15:47

thomasjpfan merged commit 0fb5295 into scikit-learn:main Jan 30, 2024
28 checks passed

This was referenced Jan 31, 2024

FEA Support missing-values in ExtraTrees* #28268

Merged

TreeRegressors with MSE Criterion do not correctly handle missing-values #28316

Closed

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Feb 10, 2024

FIX make sure to reinitialize criterion even when n_missing=0 (scikit…

162a8d9

…-learn#28295)

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Feb 13, 2024

FIX make sure to reinitialize criterion even when n_missing=0 (scikit…

bd633bb

…-learn#28295)

glemaitre added a commit that referenced this pull request Feb 13, 2024

FIX make sure to reinitialize criterion even when n_missing=0 (#28295)

92a39b8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX make sure to reinitialize criterion even when n_missing=0 #28295

FIX make sure to reinitialize criterion even when n_missing=0 #28295

FIX make sure to reinitialize criterion even when n_missing=0 #28295

FIX make sure to reinitialize criterion even when n_missing=0 #28295

Conversation

✔️ Linting Passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment