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 missing check to suppress compiler warning #1380

Merged
merged 2 commits into from Apr 30, 2018

Conversation

Projects
None yet
3 participants
@WhoisZihan
Contributor

WhoisZihan commented Apr 24, 2018

The variable 'model' may not be initialized, thus causing a compile warning in
src/mlpack/methods/hoeffding_trees/hoeffding_tree_main.cpp:251

Add missing check to suppress compiler warning
The variable 'model' may not be initialized, thus causing a compile warning in
src/mlpack/methods/hoeffding_trees/hoeffding_tree_main.cpp:251
@rcurtin

Hey there,

Thanks so much for the fix here. I think that it is not possible for the code to actually get to that point. So an easier fix would be to change the last else if to just an else. If you'd like to make that change, feel free, otherwise I will switch the code when I do the merge.

I'll go ahead and merge this in 3 days to leave time for comments. If you like, we can add your name to the list of contributors in src/mlpack/core.hpp and COPYRIGHT.txt (since even though it is a small contribution it is still a contribution!). Also, if you like, I can mail you some mlpack stickers that you can put on your laptop. If you want some, send me your mailing address at ryan@ratml.org and I will put them in the mail.

@@ -159,6 +159,10 @@ static void mlpackMain()
model = new HoeffdingTreeModel(HoeffdingTreeModel::INFO_HOEFFDING);
else if (CLI::HasParam("info_gain") && (numericSplitStrategy == "binary"))
model = new HoeffdingTreeModel(HoeffdingTreeModel::INFO_BINARY);
else {

This comment has been minimized.

@zoq

zoq Apr 25, 2018

Member

Avoid placing opening braces on the same line. See https://github.com/mlpack/mlpack/wiki/DesignGuidelines#brace-placement for more information; this might be irrelevant if we switch the else if to else.

This comment has been minimized.

@WhoisZihan

WhoisZihan Apr 26, 2018

Contributor

Thanks for clarifying, I just updated the pull request with a new commit.

Cover edge case to suppress compiler warning
The variable 'model' may not be initialized, thus causing a compile warning in
src/mlpack/methods/hoeffding_trees/hoeffding_tree_main.cpp:251
@WhoisZihan

This comment has been minimized.

Contributor

WhoisZihan commented Apr 26, 2018

@rcurtin @zoq Hi, thanks for the review.

I think that it is not possible for the code to actually get to that point. So an easier fix would be to change the last else if to just an else.

Thanks for clarifying, I didn't know it because I just start to get myself familiar with machine learning and mlpack, so this is just a minor patch without knowing the context. I just update it with a new commit.

I'll go ahead and merge this in 3 days to leave time for comments. If you like, we can add your name to the list of contributors in src/mlpack/core.hpp and COPYRIGHT.txt (since even though it is a small contribution it is still a contribution!). Also, if you like, I can mail you some mlpack stickers that you can put on your laptop. If you want some, send me your mailing address at ryan@ratml.org and I will put them in the mail.

That would be nice! Stickers will look cool. I think I can contribute more when I get familiar enough with ML and mlpack.

@WhoisZihan

This comment has been minimized.

Contributor

WhoisZihan commented Apr 26, 2018

The new commit fails travis-ci test, saying

18/149 Test #17: CLIBindingTest ...................***Failed 0.06 sec
Running 24 test cases...
/home/travis/build/mlpack/mlpack/src/mlpack/tests/cli_binding_test.cpp(123): fatal error in "GetParamUnloadedMatTest": critical check output->n_rows == 5 failed [3 != 5]
*** 1 failure detected in test suite "mlpackTest"

However, when I run bin/mlpack_test and ctest -j4 in my ubuntu 16.04 machine, all tests are passed...

11/149 Test #14: BinarizeTest ..................... Passed 0.21 sec
Start 15: BlockKrylovSVDTest
Start 17: CLIBindingTest
12/149 Test #17: CLIBindingTest ................... Passed 0.30 sec
Start 18: CLITest
13/149 Test #3: AdaGradTest ...................... Passed 6.71 sec
14/149 Test #18: CLITest .......................... Passed 0.23 sec
Start 19: CMAESTest
Start 20: CNETest
.......
148/149 Test #114: SVDBatchTest ..................... Passed 0.30 sec
Start 115: SVDIncrementalTest
149/149 Test #115: SVDIncrementalTest ............... Passed 0.48 sec

100% tests passed, 0 tests failed out of 149

Total Test time (real) = 259.86 sec

It looks strange since I never touch that test case or source file. I wonder are you having the same issue?

@zoq

This comment has been minimized.

Member

zoq commented Apr 26, 2018

Sometimes, tests that have nothing to do with your code will fail. Don't worry about those; see #922 for more information. In this case we should see if we can track down the error, but no need to delay the PR.

@zoq

zoq approved these changes Apr 26, 2018

Thanks for the contribution, no more comments from my side.

@rcurtin rcurtin merged commit f17dea2 into mlpack:master Apr 30, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Memory Checks
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment