8000 SplineTransformer raises when passing knots explicity for n_knots=None · Issue #19598 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

SplineTransformer raises when passing knots explicity for n_knots=None #19598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
mlondschien opened this issue Mar 2, 2021 · 5 comments · Fixed by #20191
Closed

SplineTransformer raises when passing knots explicity for n_knots=None #19598

mlondschien opened this issue Mar 2, 2021 · 5 comments · Fixed by #20191
Labels
Enhancement module:preprocessing

Comments

@mlondschien
Copy link
Contributor

On master,

import numpy as np

from sklearn.preprocessing import SplineTransformer

X = np.linspace(0, 1, 10)[:, None]
splt = SplineTransformer(
   knots=[[0], [1], [2], [3], [4]],
   n_knots=None,
   degree=3
)

splt.fit(X)

currently raises

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-5-86bffa93d84e> in <module>
     10 )
     11 
---> 12 splt.fit(X)

~/code/scikit-learn/sklearn/preprocessing/_polynomial.py in fit(self, X, y)
    226             isinstance(self.n_knots, numbers.Integral) and self.n_knots >= 2
    227         ):
--> 228             raise ValueError("n_knots must be a positive integer >= 2.")
    229 
    230         if isinstance(self.knots, str) and self.knots in [

ValueError: n_knots must be a positive integer >= 2.

IMO the n_knots argument should be allowed to be None (or any other value) if the knots are passed explicity.

cc: @lorentzenchr @ogrisel
xref #18368 and #19483
@lorentzenchr
Copy link
Member

Another example:

from sklearn.ensemble import GradientBoostingRegressor

g = GradientBoostingRegressor(alpha=None)
g.fit([[1]], [2])

throws an error, even if alpha is not used.

@mlondschien Why do you think n_knots=None should be allowed when knots are specified manually?

I'd be interested in the opinion of others. Do we have a general pattern that we usually follow?

@mlondschien
Copy link
Contributor Author

Currently

import numpy as np

from sklearn.preprocessing import SplineTransformer

X = np.linspace(0, 1, 10)[:, None]
splt = SplineTransformer(
   knots=[[0], [1], [2], [3], [4]],
   n_knots=2,
   degree=3
)

splt.fit(X)

works. IMO this is "more incorrect" than n_knots=None.

Raising an error that n_knots needs to be in integer when knots is passed explicitly is confusing. This suggests that the value in n_knots get's used somewhere. I would generally pass n_knots=None explicitly to make sure that the argument does not get used.

@ogrisel
Copy link
Member
ogrisel commented Mar 19, 2021

As the 2 constructor parameters are mutually exclusive, we could collapse them into the knots param which could accept either an array or an int. WDYT?

As SplineTransformer has never been part of a release yet, we can still change this without having to deal with backward compat.

@lorentzenchr
Copy link
Member
lorentzenchr commented Mar 19, 2021 •
edited
Loading

If knots in ["uniform", "quantile"], then n_knots specifies the number if knots. If knots=2d array specifying the knot positions manually, then n_knots is ignored. So they can't be collapsed, but that would have been nice!

@lorentzenchr lorentzenchr added the Needs Decision Requires decision label Mar 24, 2021
@lorentzenchr
Copy link
Member

@mlondschien I'm fine with allowing n_knots=None if the knots are specified manually. Do you want to open a PR?

@mlondschien mlondschien mentioned this issue Jun 2, 2021
Merged
@lorentzenchr lorentzenchr removed the Needs Decision Requires decision label Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement module:preprocessing
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow n_knots=None if knots are explicitly specified in SplineTransformer mlondschien/scikit-learn
4 participants
@ogrisel @cmarmo @lorentzenchr @mlondschien
0