[Feature] Decay functions #594
Conversation
sklego/meta/_decay_utils.py
Outdated
| self.n_steps = n_steps | ||
| self.step_size = step_size |
There was a problem hiding this comment.
Not a huge fan of this implementation and how it has to be handled in the .__call__(). Looking for advice 😊
There was a problem hiding this comment.
I'm not sure what you're referring to here. You mean the ValueErrors? Is there a reason why they can't be handled here in __init__?
There was a problem hiding this comment.
You are correct. Every check could be performed in the __init__; my mind just switches to sklearn framework when working on the project, and no checking is done in classes initialization.
What I am not a huge fan is having no default values at all and two mutually exclusive parameters.
There was a problem hiding this comment.
Moved validation to __init__
| trf = DecayEstimator(LinearRegression(), check_input=True) | ||
| @pytest.mark.parametrize("decay_func", ["exponential", "linear", "sigmoid"]) | ||
| def test_estimator_checks_regression(test_fn, decay_func): | ||
| trf = DecayEstimator(LinearRegression(), decay_func=decay_func, check_input=True) | ||
| test_fn(DecayEstimator.__name__, trf) |
There was a problem hiding this comment.
I couldn't manage to change default values in ln22 and ln 29 as well.
| @@ -0,0 +1,272 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
Should we add check for min_value to be strictly positive? Estimators will handle that anyway, I would say it is redundant
There was a problem hiding this comment.
In general: if there's an opportunity to make a better error message than the library under the hood it may be worth the investment. In this case, it might help to explain to the user why, given the context of what a decay function is supposed to do.
Then again, it's also ok not to add an error message for every little thing that might go wrong as well. It's a balance. Feel free to omit if you prefer :)
| def test_exponential_decay(kwargs, context): | ||
| X, y = np.random.randn(100, 10), np.random.randn(100) | ||
|
|
||
| with context: |
There was a problem hiding this comment.
TIL about does_not_raise(). Nice.
There was a problem hiding this comment.
I love such trick to save on number of test function 😉
There was a problem hiding this comment.
I guess the main thing that's missing here is perhaps a docs page with plots of all of these decay functions?
I'll leave it up to @FBruzzesi to determine if it's best to add that later given the big push on the docs. But I'd imagine without those docs people may omit this new feature, which would be a shame.
In order we could:
|
|
@FBruzzesi just merged the docs. Feel free to continue on this thread again. I'll try to figure out a nice docs deployment setup from my machine in the meantime. |
|
Ready for review!
|
Description
Introduces built-in decay functions as discussed in #581
Type of change
Checklist: