MonotonicSplineTransformer #709
Conversation
|
@matekadlicsko will present this to the larger crowd tomorrow. |
|
I believe this implementation has an error. The cumsum should go over the columns instead of the rows, which in turn makes sorting obsolete as well. I can fix this. |
|
Feel free to put a PR on top of this one. In particular, if you could contribute a unit test that demonstrates when something breaks that would be stellar. |
* fix: fixed an error in the implementation * feat: tests now directly test monotonicity and boundedness --------- Co-authored-by: Mate Kadlicsko <[email protected]>
|
Updated the docs. If tests pass the WIP status can be removed. |
|
@FBruzzesi I think all the base tests pass, so this one might be green to go in. |
|
All 3.8 tests broke, but it is also end of life, so I removed those checks. |
There was a problem hiding this comment.
Hey @koaning, I left a bunch of comments, mostly for documenting and some other minor details.
For the algorithm itself I did not do my homework properly as I did not have the chance to watch last Friday live stream, yet I trust your and Mate Kadlicsko implementation and review.
| for col in range(X.shape[1]): | ||
| out.append( | ||
| np.cumsum( | ||
| self.spline_transformer_[col].transform(X[:, [col]])[:, ::-1], |
There was a problem hiding this comment.
I may need to do a bit more reading on why there is an inversion here
There was a problem hiding this comment.
The direction in which you perform the cumsum on the columns will determine whether the output splines will be increasing or decreasing. However, now that you mention it, I'm pretty sure that
1 - np.cumsum(without inversion)
would achieve the same effect.
Co-authored-by: Francesco Bruzzesi <[email protected]>
Co-authored-by: Francesco Bruzzesi <[email protected]>
An implementation for #704