-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added more options like min_samples_split,min_samples_leaf,max_features and max_nodes in Decision Tree scikit implementation #43
Conversation
Can one of the admins verify this patch? |
methods/scikit/dtc.py
Outdated
random_state=self.seed) | ||
random_state=self.seed, | ||
splitter = self.splitter, | ||
min_samples_split = self.min_samples_split, min_weight_fraction_leaf=self.min_weight_fraction_leaf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use one line for each parameter, to improve the readability?
Done. Please Review. |
@mlpack-jenkins test this |
Not getting why the benchmark_ann file fails to compile here. Last time it was checked it compiled just fine. |
It compiles without any warnings on my local system while working on the same branch. |
@Iron-Stark Turns out the test sometimes fails when used with a small dataset, I'm not why. Let's see if testing on a bigger dataset helps. |
@mlpack-jenkins test this |
Another great contribution, thanks! |
Thanks. :) I am planning to implement approximate nearest neighbours using the NearPy library for the benchmarks . Will make a PR by tonight. |
Added more options like min_samples_split,min_samples_leaf,max_features and max_nodes in Decision Tree scikit implementation.
@rcurtin
I have added this PR in place of PR(#39) after updating my fork. Please Review.