Integration tests setup, updated docs #51
Conversation
|
Seems like |
.travis.yml
Outdated
| @@ -27,13 +27,15 @@ install: | |||
| - pip install --upgrade pip | |||
| - pip install -r requirements/requirements.txt | |||
| - pip install -r requirements/test_requirements.txt | |||
| - pip install -r requirements/integration_test_requirements.txt | |||
There was a problem hiding this comment.
To stop the linux logs reaching the size limit, maybe making the pip install less verbose would work?
| - pip install -r requirements/integration_test_requirements.txt | |
| - pip install -r requirements/integration_test_requirements.txt -q |
There was a problem hiding this comment.
That's a good idea, thanks. Will certainly give it a try.
| pip install git+https://github.com/automl/random_forest_run.git | ||
| pip install scikit-learn | ||
|
|
||
| Refer to http://scikit-learn.org/stable/install.html for further information. | ||
| """) | ||
|
|
||
|
|
||
| class RandomForest(IModel): | ||
|
|
||
| def __init__(self, X_init: np.ndarray, Y_init: np.ndarray, num_trees: int = 30, |
There was a problem hiding this comment.
how do you feel about allowing the user to pass in kwargs for the sklearn model? We don't want to expose all options in the sklearn random forest constructor but we are limiting what a user can do here.
There was a problem hiding this comment.
Personally, I feel negatively, but I can be persuaded...
Anyways, the sole point of existence of these models, from my point of view, was to give users something simple to play with out of the box. They will be limiting no doubt, but then they are not intended for a real modelling.
There was a problem hiding this comment.
But if they can be made ready for "real" modelling with such a small change, shouldn't we do it? I think it would be quite frustrating if I had to re-implement this class just so I can pass in one more parameter to the sklearn random forest constructor.
An alternative might be to have a "SciKitLearnRandomForestWrapper" and simply have a function that creates a random forest with default parameters, passes it in to the wrapper then returns this wrapper to the user. Then if a user wants to create a custom random forest model they can reuse the wrapper.
|
@marpulli I was going to get rid of |
Codecov Report
@@ Coverage Diff @@
## develop #51 +/- ##
===========================================
+ Coverage 81.2% 90.01% +8.81%
===========================================
Files 62 58 -4
Lines 1532 1382 -150
Branches 154 140 -14
===========================================
Hits 1244 1244
+ Misses 253 103 -150
Partials 35 35
Continue to review full report at Codecov.
|
|
"will increase coverage by 8.81%." yay! |
Issue #, if available: #32
Description of changes:
Testing: made sure tests, integration tests and docs all run locally
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.