[MRG] Avoid duplicated methods whenever possible + more robust parameter passing #3790
Conversation
|
@pprett Are there reasons to keep the bunch of single use methods such (_init_state, _clear_state, _resize_state, _is_initialized, _fit_stages, _check_params, _fit_stage,...)? It makes the code very hard to read in my opinion. |
| yield self.classes_.take(decisions, axis=0) | ||
| else: | ||
| for y in self.staged_decision_function(X): | ||
| yield y.ravel() |
There was a problem hiding this comment.
I've actually been removing this idiom in various places (maybe this exact module). It defeats the use of inheritance by mixing the subclasses' logic in the base class.
If you want to de-duplicate the docstrings, you can use the template method pattern:
def staged_predict(self, x):
"""docstring"""
return self._staged_predict(x)This way, if you want to understand one subclass's logic, you don't have to disentangle it from the other's.
There was a problem hiding this comment.
Ok, I will revert the change for the predict functions. However, I don't think it's worth to have separate fit function.
…on and regression
|
@larsmans Do you have more comments? |
|
@pprett any more comments? |
|
sorry -- missed the PR @arjoly |
|
LGTM -- +100 for using keyword arguments over positional args -- thanks @arjoly |
|
Pulled, squashed and pushed. Thanks @arjoly, sorry to have kept you waiting! |
|
Great :-) Happy that this is merged! |
|
Thanks for the review! |
The main goal of this pr is to ease maintenance.