ENH Add support for np.nan values in SplineTransformer #28043

StefanieSenger · 2024-01-02T12:05:59Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Adds support for np.nan values in SplineTransformer.

adds param handle_missing : {'error', 'zeros'} to init, where error preserves the previous behaviour and zeros handles nan values by setting their spline values to all 0s
adds new tests

(very outdated, should have put it in a separate comment:)
~~Yet to solve:~~

I believe in _get_base_knot_positions I have to prepare _weighted_percentile for excluding nan values similarity to how np.nanpercentile excludes nan values for the calculation of the base knots. I tried, but it was quite tricky. Edit: Just found that np.nanpercentile will have a sample_weight option soon: PR 24254 in numpy

~~Should an error also be raised in case the SplineTransformer was instantiated with (handle_missing="error"), then fitted without missing values and the X then contains missing values in transform?~~

github-actions · 2024-01-02T12:07:14Z

✔️ Linting Passed

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

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

ogrisel

The PR looks very good but it needs to be merged with main (there are conflicts in the changelog).

Also, I think the get_output_feature_names() method needs to be updated. The tests should be expanded accordingly, maybe to also include a test with .set_output(transform="pandas") (this is how I found out that there was a problem with the output feature names).

ogrisel · 2024-02-29T09:59:47Z

So, should sparse_output=True and handle_missing="indicator" be prevented from being used together (and show an explicit error message) ,

I think we should add support for using those two options together.

ogrisel

Here is a more in depth pass of review. There is indeed a fundamental problem with the current code: the missingness indicators from the training set (when calling .fit or .fit_transform) should not be stored as an estimator attribute and reapplied to the test set (when calling .transform). Instead the missingness pattern from the test set should be extracted.

See more details below:

sklearn/preprocessing/_polynomial.py

ogrisel · 2024-03-01T10:22:41Z

sklearn/preprocessing/_polynomial.py

        if self.include_bias:
-            return XBS
+            return self._concatenate_indicator(XBS)


The missingness indicators computed from the X passed to .transform (which can be a test set) should be passed as argument to _concatenate_indicator instead of reusing the mask extracted from the training set.

sklearn/preprocessing/tests/test_polynomial.py

StefanieSenger

Hey @ogrisel, thanks for reviewing and your help.
I went through your comments and could resolve most of the issues.

I've named the new option handle_missing="constant", but that's just an idea. I found that indicator doesn't fit so well anymore, if we don't add an indicator column to X. Though with constant as well as with zeros I feel that it's not quite clear from the naming, where in the process the nans become something else (before or after calculating the splines). Maybe we can find a name, that conveys that info.

There are quite a few things, I am a bit confused about:
Generally, I don't know if we want SplineTransformer to change or keep behaviour if nan values are present.

If we want it to keep behaviour, instead of having this test data for comparing equality:

    X_nan = np.array([[1, 1], [2, 2], [3, 3], [np.nan, 4], [4, 4]])
    X = np.array([[1, 1], [2, 2], [3, 3], [4, 4]])

it should maybe rather be

    X_nan = np.array([[1, 1], [2, 2], [3, 3], [np.nan, 4], [4, 4]])
    X = np.array([[1, 1], [2, 2], [3, 3], [99, 4], [4, 4]])

and in this case, the current implementation is wrong. Maybe you can shed a light on this so that I know how to go on.

I will check the issue with the feature names next.

sklearn/preprocessing/_polynomial.py

sklearn/preprocessing/tests/test_polynomial.py

StefanieSenger · 2024-03-15T13:41:50Z

I was trying to find about the problem with the feature names, that you have mentioned here, @ogrisel, but I cannot recreate it. Maybe it's been resolved when I worked on the other issues?

This is what I tried (using the code from the existing feature_name_out test):

def test_spline_transformer_feature_names_with_nans():
    """Test that SplineTransformer generates correct feature names if nan values are present."""
    X_nan = np.array([[1, 1], [2, 2], [3, 3], [np.nan, 4], [4, 4]])
    splt = SplineTransformer(
        degree=3,
        n_knots=3,
        handle_missing="constant",
        include_bias=True).fit(X_nan)
    feature_names = splt.get_feature_names_out()
    assert_array_equal(
        feature_names,
        [
            "x0_sp_0",
            "x0_sp_1",
            "x0_sp_2",
            "x0_sp_3",
            "x0_sp_4",
            "x1_sp_0",
            "x1_sp_1",
            "x1_sp_2",
            "x1_sp_3",
            "x1_sp_4",
        ],
    )

    splt = SplineTransformer(
    degree=3,
    n_knots=3,
    handle_missing="constant",
    include_bias=False).fit(X_nan)
    feature_names = splt.get_feature_names_out(["a", "b"])
    assert_array_equal(
        feature_names,
        [
            "a_sp_0",
            "a_sp_1",
            "a_sp_2",
            "a_sp_3",
            "b_sp_0",
            "b_sp_1",
            "b_sp_2",
            "b_sp_3",
        ],
    )

    splt.set_output(transform="pandas")
    X_transformed = splt.transform(X_nan)
    feature_names = splt.get_feature_names_out(["a", "b"])
    assert_array_equal(
        feature_names,
        [
            "a_sp_0",
            "a_sp_1",
            "a_sp_2",
            "a_sp_3",
            "b_sp_0",
            "b_sp_1",
            "b_sp_2",
            "b_sp_3",
        ],
    )

Everything behaves as it should, I believe. But also maybe I didn't understand what you exactly ran into.

ogrisel · 2024-03-15T17:42:34Z

The get_feature_names problem only happens when appending extra output features for the missing indicators (they would then need to be named in that case).

EDIT: I will try to answer your other questions/comments early enough next week.

ogrisel · 2024-03-15T17:45:32Z

About handle_missing="constant", I really prefer handle_missing="zero" or handle_missing="zeros" or handle_missing="ignore" to either convey that missing values are encoded as zero spline feature values or are encoded in a value such that a simple downstream linear model would basically "ignore" them.

StefanieSenger · 2024-04-18T14:05:42Z

Hey @ogrisel, can you give me some feedback?

My current understanding is that if we introduce new 0-values in X_transformed (due to nan values in X), then we also expect different stats for the transformer compared to when no nan values are present.

This would mean, that we expect (and test for)

a different output format in case of nans (I have already written the tests like this)
that the B-splines won't sum up to one * n_features (that I need to define for the test)

ogrisel · 2025-04-18T15:42:45Z

I have the feeling that this PR is nearly ready to get in, so I milestoned it for 1.7 but no problem if not ready in time.

StefanieSenger · 2025-04-18T22:23:15Z

Hi, @ogrisel. Thanks for revisiting this PR and for the milestone assignment for version 1.7.

For context, I had kept this PR updated, made it pass all tests and ensured it was merge-ready, but after several months without much engagement from the team, I shifted my attention to other tasks, as the priority wasn’t clear.

Now I'm currently on vacation and will take this back up on my return. I look forward to finalizing it together.

…error messages

StefanieSenger · 2025-05-15T19:18:48Z

Hi @ogrisel, I have worked on what you had proposed and the tests now all pass.
Do you want to have a look?

StefanieSenger · 2025-05-16T11:07:52Z

sklearn/preprocessing/_polynomial.py

+
+    def __sklearn_tags__(self):
+        tags = super().__sklearn_tags__()
+        tags.input_tags.allow_nan = self.handle_missing == "zeros"


This way of defining a tag for nan support is per instance, which I think is most user-friendly and is also needed to pass all the common tests. With tags.input_tags.allow_nan = True (or False) either one fails, so we cannot do that.

However, doc/sphinxext/allow_nan_estimators.py that allows to automatically generate a list of estimators is doing it with the default parameter settings (almost as if the tags were defined at class level).
I think I would like to work on a follow-up PR to fix the generation and take cases like this into account. The question would be how large to span that. What do you think, @adrinjalali?

It's a bit sad indeed. But I think we can live with the fact that this list only reflects the default behavior, and that some estimators can be made to accept nans with specific hyper-parameters.

I think we should make sure _construct_instances creates at least one instance where it supports nan, and instead of checking only the first instance returned by the generator, check them all here

scikit-learn/doc/sphinxext/allow_nan_estimators.py

Lines 22 to 25 in 675736a

# Here we generate the text only for one instance. This directive

# should not be used for meta-estimators where tags depend on the

# sub-estimator.

est = next(_construct_instances(est_class))

But certainly for a separate PR.

ogrisel

LGTM. Thank you very much for the final push @StefanieSenger!

ogrisel · 2025-05-16T13:20:37Z

cc @lorentzenchr for a second review.

lorentzenchr

Overall looks good.
Feedback: Review was a bit harder than necessary due to renaming of variables.

lorentzenchr · 2025-05-19T13:28:07Z

sklearn/preprocessing/_polynomial.py

+        Specifies the way missing values are handled.
+
+        - 'error' : Raise an error if `np.nan` values are present during :meth:`fit`.
+        - 'zeros' : Encode missing values as splines with value `0`.


It would be nice to add a short info that this is different from first setting feature values to zero and then create the spline basis, i.e. pipeline(SimpleImputer(strategy="constant", fill_value=0), SplineTransformer(...)).

I agree it's important. I've added a note.

Similarly, Encode missing values as splines with value 0. was not so clear. I've corrected with Encode splines of missing values with values 0.

sklearn/preprocessing/_polynomial.py

lorentzenchr · 2025-05-19T13:43:42Z

sklearn/preprocessing/_polynomial.py

+                    "`X` contains invalid values (NaNs) and `SplineTransformer` is "
+                    "configured with `handle_missing='error'`. Set "
+                    "`handle_missing='zeros'` to encode missing values as splines with "
+                    "value `0` or ensure no missing values in `X`."


Suggested change

"`X` contains invalid values (NaNs) and `SplineTransformer` is "

"configured with `handle_missing='error'`. Set "

"`handle_missing='zeros'` to encode missing values as splines with "

"value `0` or ensure no missing values in `X`."

"Input X contains NaN values and `SplineTransformer` is configured"

"to error in this case (handle_missing='error'). To avoid this error, you"

"could set handle_missing='zeros' to encode missing values as splines with "

"value 0 or ensure no missing values in X."

Please tell our fellow scikit-learn (core) contributors to stop using backticks in printed output, error and warning messages as well as code comments.
Disclaimer: I might have introduced some of those in the past.

Why is that? My impression was that maintainers encourage the use of backticks in error messages, or did I get this wrong?

I don’t like it. Whether others agree should be settled elsewhere. For this PR, let’s not introduce new backticks.

we're certainly not consistent with backticks and some people use them more than others. I personally don't bother asking people to add or remove them.

sklearn/preprocessing/_polynomial.py

lorentzenchr · 2025-05-19T14:47:51Z

sklearn/preprocessing/tests/test_polynomial.py

+        sparse_output=sparse_output,
+    )
+
+    # Check `fit_transform` does the same as `fit` and then `transform`:


Suggested change

# Check `fit_transform` does the same as `fit` and then `transform`:

# Check fit_transform does the same as fit and then transform:

While I'm sorry for the noise, I don't like backticks in comments. It does not help reading to me.

I personally like them as they help disambiguate the named API entities from the English verbs, especially in sentences that would otherwise be grammatically incorrect, as is the case in this particular snippet.

Backticks for named entities also help me read comments more intuitively. Will it be okay for you to keep them here, @lorentzenchr?

sklearn/preprocessing/tests/test_polynomial.py

lorentzenchr · 2025-05-19T14:51:30Z

sklearn/preprocessing/tests/test_polynomial.py

+    # Check that `transform` works as expected when the passed data has a different
+    # shape than the training data passed to `fit`:


Suggested change

# Check that `transform` works as expected when the passed data has a different

# shape than the training data passed to `fit`:

# Check that transform works as expected when the passed data has a different

# shape than the training data passed to fit:

sklearn/preprocessing/tests/test_polynomial.py

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

…kit-learn into nan_SplineTransformer

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

StefanieSenger

Thank you @lorentzenchr. I have implemented your suggestions and left comments on two I would rather like to leave as they are (the copy() and the backticks). See my comments for reasoning.

StefanieSenger · 2025-06-11T12:21:15Z

sklearn/preprocessing/_polynomial.py

+        Specifies the way missing values are handled.
+
+        - 'error' : Raise an error if `np.nan` values are present during :meth:`fit`.
+        - 'zeros' : Encode missing values as splines with value `0`.


I agree it's important. I've added a note.

Similarly, Encode missing values as splines with value 0. was not so clear. I've corrected with Encode splines of missing values with values 0.

StefanieSenger · 2025-06-11T12:26:37Z

sklearn/preprocessing/tests/test_polynomial.py

+        sparse_output=sparse_output,
+    )
+
+    # Check `fit_transform` does the same as `fit` and then `transform`:


Backticks for named entities also help me read comments more intuitively. Will it be okay for you to keep them here, @lorentzenchr?

sklearn/preprocessing/_polynomial.py

StefanieSenger · 2025-06-11T12:49:20Z

sklearn/preprocessing/_polynomial.py

-                    # later.
-                    x[mask_inv] = spl.t[self.degree]
+                    outside_range_mask = ~inside_range_mask
+                    x = X[:, feature_idx].copy()


I think we do need the copy. That was actually pretty hard for me to figure out, because I was not familiar with views on arrays back then and I had naively implemented everything without a copy. I have also checked again: Without a copy, we're changing X with x[outside_range_mask] = xmin, and it matters, because earlier we rely on X.

(I think it is in

x = spl.t[spl.k] + (X[:, feature_idx] - spl.t[spl.k]) % ( spl.t[n] - spl.t[spl.k]

above.)

We get a lot of errors too without the copy.

StefanieSenger · 2025-06-11T14:49:38Z

sklearn/preprocessing/_polynomial.py

-            # Note for extrapolation:
-            # 'continue' is already returned as is by scipy BSplines


That's right. I have put this comment back in. I had originally deleted it because I had thought it was wrong, but now can clearly see the splines are returned as is from scipys BSpline.

StefanieSenger · 2025-06-11T17:02:08Z

sklearn/preprocessing/_polynomial.py

+                    # Replace any output that corresponds to a missing input value
+                    # by zero.
+                    for spline_idx in range(n_splines):
+                        output_feature_idx = n_splines * feature_idx + spline_idx
+                        XBS[
+                            nan_row_indices, output_feature_idx : output_feature_idx + 1
+                        ] = 0


I have tried to do this in this commit, the tests seem to pass, but I'm not very happy about the result. It looks even more confusing.

Ideally, for me, this transform method would be structured regardless of eventual repetition of code lines - something either like:

if use_sparse: if extrapolation == "continue": ... if extrapolation == "constant": ... if extrapolation == "error": ... ... else: if extrapolation == "continue": ... if extrapolation ...

or like:

if extrapolation == "continue": if use_sparse: ... else: ... if extrapolation == "constant": if use_sparse: ... else: ... if extrapolation == "error": ... ...

(So it would have to main branches, either on use_sparse first or on extrapolation first, and be fully explicit inside each.)

I didn't manage to make this refactoring in this same PR, that was already challenging enough as it was. Probably, because I hadn't discovered all the spots where the code is zipped together across control flow branches (like I did in the last commit, if that makes sense). Next time I would touch something like that, I would make sure to try some refactoring before starting to add new functionality.

Please let me know what you think of this commit, @lorentzenchr, as I am not sure if it easier to read.

StefanieSenger · 2025-06-11T17:02:32Z

sklearn/preprocessing/_polynomial.py

                if use_sparse:
+                    # Copy the current column to avoid mutation of the user provided


I am not 100% sure if this comment refers to the same dealing with x as this comment? It probably doesn't, right? If so, then I don't understand and didn't address it. In this case, could you please elaborate, @lorentzenchr?

StefanieSenger added 2 commits December 20, 2023 13:32

added indicator

4255470

added splines full of 0 for nan input in transform

812ae00

github-actions bot added the module:preprocessing label Jan 2, 2024

StefanieSenger and others added 2 commits January 8, 2024 11:39

Merge branch 'main' into nan_SplineTransformer

0ad4f5e

fixed extrapolation='error' related error

f0f7d5e

StefanieSenger changed the title ~~ENH Adds support for np.nan values in SplineTransformer~~ ENH Add support for np.nan values in SplineTransformer Jan 8, 2024

StefanieSenger added 4 commits January 9, 2024 14:14

fixed _get_base_knot_positions so spline parameters are build correctly

a38ee69

exclude knots='quantile' from camparison check

4c9a867

fixed issues from test_common

f7f7ca4

little things

4bbdd4a

ogrisel mentioned this pull request Feb 28, 2024

Handle np.nan / missing values in SplineTransformer #26793

Open

ogrisel reviewed Feb 28, 2024

View reviewed changes

ogrisel mentioned this pull request Feb 28, 2024

Implement SplineTransformer.inverse_transform #28551

Open

Merge branch 'main' into nan_SplineTransformer

092eca0

ogrisel reviewed Mar 1, 2024

View reviewed changes

StefanieSenger and others added 3 commits March 13, 2024 16:22

calculate indicator based on X passed into transform

5165c6b

Merge branch 'main' into nan_SplineTransformer

1a7d134

add and adjust tests after review

708ae53

StefanieSenger commented Mar 15, 2024

View reviewed changes

sklearn/preprocessing/_polynomial.py Show resolved Hide resolved

sklearn/preprocessing/_polynomial.py Outdated Show resolved Hide resolved

sklearn/preprocessing/tests/test_polynomial.py Show resolved Hide resolved

sklearn/preprocessing/tests/test_polynomial.py Outdated Show resolved Hide resolved

wording and typo

1dc3b3c

Merge branch 'main' into nan_SplineTransformer

dce1620

adrinjalali mentioned this pull request Apr 15, 2024

WIP KBinsDiscretizer supports NaN as input values #19928

Open

Merge branch 'main' into nan_SplineTransformer

63ea8aa

StefanieSenger marked this pull request as ready for review April 18, 2024 13:51

ogrisel added this to the 1.7 milestone Apr 18, 2025

StefanieSenger and others added 8 commits May 6, 2025 11:30

update changelog

f6ab8f8

improve comments

3f47364

remove erroring behavior for nan and sample_weight and improve other …

656685b

…error messages

fix input_tags for nans

6571a12

add test for sample_weight and nan value handling

a41deb1

fix exception handling for nan input

6abb0af

fix shape mismatch in CI due to sparse-dense conversion

038ffe8

Merge branch 'main' into nan_SplineTransformer

fe27494

lucyleeow mentioned this pull request May 16, 2025

Inconsistent median/quantile behaviour now _weighted_percentile ignores NaNs #31367

Closed

StefanieSenger commented May 16, 2025

View reviewed changes

ogrisel modified the milestones: 1.7, 1.8 May 16, 2025

ogrisel approved these changes May 16, 2025

View reviewed changes

lorentzenchr approved these changes May 19, 2025

View reviewed changes

StefanieSenger and others added 8 commits May 20, 2025 15:39

Apply suggestions from code review

4b0cb85

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

fix formatting

c547dc6

Apply suggestions from code review

4e430c3

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

Merge branch 'nan_SplineTransformer' of github.com:StefanieSenger/sci…

b54dfb2

…kit-learn into nan_SplineTransformer

improve documentation

7e50b73

Merge branch 'main' into nan_SplineTransformer

8babd2e

Apply suggestions from code review

a1bf164

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>

refactor

2150866

StefanieSenger commented Jun 11, 2025

View reviewed changes

StefanieSenger added 2 commits June 12, 2025 09:12

Merge branch 'main' into nan_SplineTransformer

6f20b52

Merge branch 'main' into nan_SplineTransformer

2b37259

	# Here we generate the text only for one instance. This directive
	# should not be used for meta-estimators where tags depend on the
	# sub-estimator.
	est = next(_construct_instances(est_class))

	# Check `fit_transform` does the same as `fit` and then `transform`:
	# Check fit_transform does the same as fit and then transform:

		# Check that `transform` works as expected when the passed data has a different
		# shape than the training data passed to `fit`:

		# Note for extrapolation:
		# 'continue' is already returned as is by scipy BSplines

		if use_sparse:
		# Copy the current column to avoid mutation of the user provided

Uh oh!

ENH Add support for np.nan values in SplineTransformer #28043

Are you sure you want to change the base?

ENH Add support for np.nan values in SplineTransformer #28043

Uh oh!

Conversation

StefanieSenger commented Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Uh oh!

github-actions bot commented Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

ogrisel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Feb 29, 2024

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StefanieSenger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StefanieSenger commented Mar 15, 2024

Uh oh!

ogrisel commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Mar 15, 2024

Uh oh!

StefanieSenger commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Apr 18, 2025

Uh oh!

StefanieSenger commented Apr 18, 2025

Uh oh!

StefanieSenger commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel commented May 16, 2025

Uh oh!

lorentzenchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

StefanieSenger commented Jan 2, 2024 •

edited

Loading

github-actions bot commented Jan 2, 2024 •

edited

Loading

ogrisel left a comment •

edited

Loading

ogrisel commented Mar 15, 2024 •

edited

Loading

StefanieSenger commented Apr 18, 2024 •

edited

Loading

StefanieSenger Jun 11, 2025 •

edited

Loading