Closed mralbu closed 3 years ago
Most of my comments have been addressed properly. Three inline comments are still open. In addition, the behavior of the class should be documented here. The code snippet from the top would be perfect.
Just checked the test coverage with nosetests --with-coverage
(results are located in cover/index.html). We should add additional tests for the missing branches:
if X.ndim == 1:
X = np.expand_dims(X, 1)
if y.ndim == 1:
y = np.expand_dims(y, 1)
etc.
Ping @mralbu
My plan is to merge #28 before this one. Do you have time to address my suggestions or should I take over?
Hi, @AlexanderFabisch
Sorry for taking this long.. I've noticed that the shape checks for X were covered by sklearn.utils.check_X_y. I haven't modified the README.rst instructions, I think you might do a better job than me. I just became aware of the issues with the scikit-learn boston dataset, so maybe another dataset would be a better choice for examples. Feel free to take over additional modifications. Thanks for reviewing and accepting this PR! :)
Sorry for taking this long..
No problem.
I haven't modified the README.rst instructions, I think you might do a better job than me.
OK, I will have it on my list.
I just became aware of the issues with the scikit-learn boston dataset, so maybe another dataset would be a better choice for examples.
There is fetch_california_housing. This dataset should be similar, right?
I merged this pull request to master. Thanks for your contribution @mralbu
I also added an example with sklearn's learning curve: https://github.com/AlexanderFabisch/gmr/blob/master/examples/plot_sklearn_learning_curve.py
Adds a scikit-learn RegressorMixin. Addressing #27. Sample usage proposed: