Skip to content
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

Add new paramter Maximum_depth to DTree And Random Forest #1916

Merged
merged 9 commits into from Jun 21, 2019

Conversation

@Yashwants19
Copy link
Contributor

commented Jun 1, 2019

Address #1910

@Yashwants19 Yashwants19 force-pushed the Yashwants19:dtree branch from 9d109f9 to 459e2a0 Jun 1, 2019
@Yashwants19

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

Hi @rcurtin , @zoq : This is ready for review.
Thank you :)

@yanivbi

This comment has been minimized.

Copy link

commented Jun 4, 2019

I executed a prediction over an RF model with configured maximum_depth=5.
The output is:
[INFO ] input_model_file: shuttle.rf-model-maxD-5.bin
[INFO ] labels_file:
[INFO ] maximum_depth: 20
[INFO ] minimum_gain_split: 0

Is it a print mistake?

Copy link
Member

left a comment

Hey @Yashwants19, thanks for addressing this so quickly! I left a few comments.

@rcurtin rcurtin removed the s: unanswered label Jun 9, 2019
Copy link
Member

left a comment

@Yashwants19 thanks for the hard work on this; I left some more comments. I think it is getting close to ready. 👍

Yashwants19 added 4 commits Jun 17, 2019
@Yashwants19

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Hi @rcurtin this ready for review. I have made the changes as suggested.

Copy link
Member

left a comment

@Yashwants19: nice work! Everything looks good to me. If you can take care of the little cleanup I suggested that would be great, otherwise I can do it during merge. Want to add a note to HISTORY.md also?

@yanivbi: I tried to reproduce what you wrote but I see the following:

$ bin/mlpack_random_forest -t vc2.csv --maximum_depth 2 -l vc2_labels.txt -v
[WARN ] Should pass one of '--test_file (-T)', '--output_model_file (-M)', or '--print_training_accuracy (-a)'; the trained forest model will not be used or saved!
[INFO ] Loading 'vc2.csv' as CSV data.  Size is 6 x 207.
[INFO ] Loading 'vc2_labels.txt' as raw ASCII formatted data.  Size is 207 x 1.
[INFO ] Training random forest with 10 trees...
[INFO ] 
[INFO ] Execution parameters:
[INFO ]   help: 0
[INFO ]   info: 
[INFO ]   input_model_file: 
[INFO ]   labels_file: vc2_labels.txt
[INFO ]   maximum_depth: ***2***

so the printed value of maximum_depth matches what I passed as an option. Perhaps this is because of a recent code change to this PR, but in any case, it looks like the issue is fixed. If you want to try again with this branch and see if the issue persists, that would be great. But if you don't have a chance before this is merged, no problem---we can always open a new issue if there is still a problem. :)


// If the gain is the best possible, no need to keep looking.
if (bestGain >= 0.0)
break;
}

This comment has been minimized.

Copy link
@rcurtin

rcurtin Jun 19, 2019

Member

Actually, you could close the if (maximumDepth != 1) block here, and then the rest of the code would work as-is: bestDim is still set to datasetInfo.Dimensionality() in this code even if maximumDepth == 1, so the block of code that computes the leaf probabilities would still be reached. Let me know if I can clarify what I mean.

This comment has been minimized.

Copy link
@Yashwants19

Yashwants19 Jun 20, 2019

Author Contributor

I have made the changes as suggested. :) and also update the HISTORY.md

src/mlpack/tests/decision_tree_test.cpp Outdated Show resolved Hide resolved
@mlpack-bot
mlpack-bot bot approved these changes Jun 20, 2019
Copy link

left a comment

Second approval provided automatically after 24 hours. 👍

Yashwants19 added 3 commits Jun 20, 2019
@Yashwants19

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Hi @rcurtin I have made the changes as suggested.
Thank You :)

@rcurtin rcurtin merged commit 6328263 into mlpack:master Jun 21, 2019
6 checks passed
6 checks passed
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

@Yashwants19 thanks again. I made some slight style and spelling fixes in e3989bd. 👍

@Yashwants19 Yashwants19 deleted the Yashwants19:dtree branch Jun 21, 2019
@Yashwants19

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Hi @rcurtin now can I create a new PR for the cli-bindings for linear_svm?

@rcurtin

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Sure, feel free. Like I said I'd like to review the Go bindings, but I'm about to be traveling for two weeks so it may be some time until I can get to those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.