koaning / scikit-lego

Extra blocks for scikit-learn pipelines.
https://koaning.github.io/scikit-lego/
MIT License
1.28k stars 117 forks source link

[BUG] `DeadZoneRegressor` with "constant" effect #587

Closed FBruzzesi closed 1 year ago

FBruzzesi commented 1 year ago

Description

DeadZoneRegressor allows for the parameter effect="constant". However such case is not managed in the deadzone function, which leads to return a value None and breaking the API rightafter.

I am not sure what the expected behaviour is. My guess would be something like:

if self.effect == "constant":
    return np.where(errors > self.threshold, some_constant_value, np.zeros(errors.shape))

However such some_constant_value is nowhere defined.

Code to reproduce

from sklego.linear_model import DeadZoneRegressor
from sklego.datasets import load_chicken

X, y = load_chicken(return_X_y=True)
model = DeadZoneRegressor(effect="constant")

_ = model.fit(X, y)

TypeError: unsupported operand type(s) for /: 'NoneType' and 'int'

Unit test to catch the bug

It should be enough to add "constant" to the list of parameter in the fixture

koaning commented 1 year ago

That indeed feels like an omission. Feel free to make the PR.

One thing: I wonder if this component is used much. One downside of the implementation is that it's pretty darn slow.

FBruzzesi commented 1 year ago

However such some_constant_value is nowhere defined.

Doesn't matter the value, right? Can just use 1, it just a matter of scale.

One downside of the implementation is that it's pretty darn slow.

Any big reason not to move to scipy.optimize.minimize? I understand the original intent was to track all loss values, weights and gradients.

One thing: I wonder if this component is used much

For sure the effect="constant" was not used 😂

koaning commented 1 year ago

Doesn't matter the value, right? Can just use 1, it just a matter of scale.

Yep!

Any big reason not to move to scipy.optimize.minimize?

I'm certainly open to it. But I'm not 100% sure if there are any numerical issues that we might hit. That said: if it works out it would be nice to drop a dependency.

For sure the effect="constant" was not used 😂

Yep! 😅