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

Option consistency in logistic regression #1565 #1616

Merged
merged 13 commits into from Feb 6, 2019

Conversation

Projects
None yet
4 participants
@Hemal-Mamtora
Copy link
Contributor

commented Dec 26, 2018

With respect to issue #1565 :
I have made changes to the logistic_regression_main.cpp
I have tested the binding for renamed parameters/options.

@rcurtin kindly review the changes. Let me know if any updates are needed.

Thank you

@ShikharJ

This comment has been minimized.

Copy link
Member

commented Dec 27, 2018

I'm not sure if these changes are necessary. @rcurtin What do you think?

@rcurtin

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

@ShikharJ: I think it's useful and should be merged. In #1565 we found that there is inconsistency in option names that would be nice to fix. 👍 Let me review it now...

@rcurtin
Copy link
Member

left a comment

Hi @Hemal-Mamtora; thanks for the contribution! It looks like this will work properly, but we should also test in src/mlpack/tests/main_tests/logistic_regression_test.cpp that the predictions and probabilities outputs work correctly, instead of the old output and output_probabilities names.

If you can handle the comments I've left, I think that we can merge this. 👍

@ShyamPandya

This comment has been minimized.

Copy link

commented Jan 3, 2019

@Hemal-Mamtora I had made the exact same changes but I could not test the binding even after building it from source. Could you help me understand how to test the new options? My email ID is shyampandya97@gmail.com

Thanks.

@Hemal-Mamtora

This comment has been minimized.

Copy link
Contributor Author

commented Jan 5, 2019

but we should also test in src/mlpack/tests/main_tests/logistic_regression_test.cpp that the predictions and probabilities outputs work correctly, instead of the old output and output_probabilities names.

@rcurtin, What I understand is, at this point of time, the tests for all 4: output,predictions , output_probabilities and probabilities should work.

Or is it the case that the 2 new options tests should work instead of the 2 old options.
Please clarify.

And please review the further changes that I have done.

@rcurtin
Copy link
Member

left a comment

@rcurtin, What I understand is, at this point of time, the tests for all 4: output,predictions , output_probabilities and probabilities should work.

Or is it the case that the 2 new options tests should work instead of the 2 old options.
Please clarify.

For the sake of reverse compatibility, all four of these outputs should work. 👍

Once those new tests are ready and working and you can handle the other comments I left, I think this is ready for merge. Thanks for the hard work!

@Hemal-Mamtora

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

@rcurtin , I don't understand why this build fails: continuous-integration/travis-ci/pr — The Travis CI build failed

Since the changes had built successfully on my local pc.

The details of the failed build are :
/home/travis/build/mlpack/mlpack/src/mlpack/tests/main_tests/logistic_regression_test.cpp(120): fatal error in "LRPredictionSizeCheck": critical check testY.n_cols == M failed [0 != 15] unknown location(0): fatal error in "LRResponsesRepresentationTest": memory access violation at address: 0x7ffd5e51c000: no mapping at fault address /home/travis/build/mlpack/mlpack/src/mlpack/tests/main_tests/logistic_regression_test.cpp(142): last checkpoint

It looks like predictions is not getting set.

Could you guide me on how to solve this issue?
Please also have a look at src/mlpack/tests/main_tests/logistic_regression_test.cpp

Thank you for your guidance !

@rcurtin

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

You can't do std::move() twice---once you call std::move() the first time, the object no longer has any data in it. That's why in the example snippet I posted I only used std::move() in the second call. If you fix that, I think that the tests will succeed.

@Hemal-Mamtora

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@rcurtin , I updated the std::move related code. Still one fatal error remains which is as follows:

unknown location(0): fatal error in "LRResponsesRepresentationTest": memory access violation at address: 0x7ffd0615f000: no mapping at fault address

Memory access violation, Please let me know what I have missed.

Thank you !

@rcurtin

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Hi @Hemal-Mamtora, what have you tried to debug the issue?

Hemal-Mamtora added some commits Jan 23, 2019

@Hemal-Mamtora

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

@rcurtin

Regarding the debugging of the issue,
I tried :

  1. Keeping separate test cases for 'predictions' and 'output' parameter
  2. Then keeping only the 'predictions' parameter for certain test cases
  3. Replacing all the 'output' parameter by 'predictions' parameter.

All these changes can be found on the above commits

Some new fatal errors have cropped up. It says:
[FATAL] Parameter '--probabilites' does not exist in this program.

But according to me the parameter 'probabilities' has no role in the logistic_regression_test.cpp
Since it's replacement 'output_probabilities' parameter is not seen in the file : logistic_regression_test.cpp

Please let me know on how to tackle this issue.

Thanks !

P.s. just ignore that I accidentally closed the Pull request while typing and had to reopen it

@Hemal-Mamtora Hemal-Mamtora reopened this Jan 24, 2019

@mlpack-bot

This comment has been minimized.

Copy link

commented Jan 24, 2019

Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:

  • All code should follow the style guide
  • Documentation added for any new functionality
  • Tests added for any new functionality
  • Headers and license information added to the top of any new code files
  • HISTORY.md updated if the changes are big or user-facing
  • All CI checks should be passing

Thank you again for your contributions! 👍

@rcurtin

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

I think you have a typo in your code :)

[FATAL] Parameter '--probabilites' does not exist in this program.

Shouldn't that be probabilities not probabilites? The problem seems to be on line 183.

@rcurtin
Copy link
Member

left a comment

Looks good to me and I can approve it but I'd like to ask if you can add one more test first---can you check that the outputs output and predictions are the same, and that output_probabilities and probabilities are the same? You can write a new test case for this, and add a comment that it can be removed for mlpack 4 when we remove the output and output_probabilities option.

Thanks for the hard work with this. 👍

@Hemal-Mamtora

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2019

OK, I am working on it!
Thank you for your review !

@Hemal-Mamtora

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

@rcurtin , I have added the 2 test cases to verify that the output from the old and the new parameters is the same. Please review. And please let me know if any changes are needed.
Thank you !

@rcurtin

rcurtin approved these changes Feb 4, 2019

Copy link
Member

left a comment

Hey @Hemal-Mamtora, thanks for sticking with this one and seeing it through to completion. 👍 The code looks good to me and I think it's ready to merge. I pointed out a few really trivial style issues that you can fix before merge, but if you don't get to it then I'll handle it during merge. The new test cases look good too. Thanks again!

PRINT_PARAM_STRING("output") + " parameter."
PRINT_PARAM_STRING("predictions") + " parameter." +
"\n\n"
"Note : The following parameters are deprecated and "

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 4, 2019

Member

This is a tiny issue but we can remove the space here so it reads Note: :)

"will be removed in mlpack 4: " + PRINT_PARAM_STRING("output") +
", " + PRINT_PARAM_STRING("output_probabilities") +
"\nUse " + PRINT_PARAM_STRING("predictions") + " instead of " +
PRINT_PARAM_STRING("output") + "\nUse " +

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 4, 2019

Member

We should add a period to the end of each of these lines, so instead of "\nUse ", maybe ".\nUse ". :)

}

/**
* Ensuring that the parameter 'output_probabilities' and the parameter

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 4, 2019

Member

This is also pretty trivial but there should only be one space before the * here.

@mlpack-bot

mlpack-bot bot approved these changes Feb 5, 2019

Copy link

left a comment

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit 0e4de2e into mlpack:master Feb 6, 2019

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 Feb 6, 2019

Thanks for the contribution! I have another bug to work out with mlpack-bot... it is supposed to offer you stickers after merge. If it did, it would say this:

Hello there!  Thanks for your contribution.  I see that this is your first contribution to
mlpack.  If you'd like to add your name to the list of contributors in `src/mlpack/core.hpp`
and `COPYRIGHT.txt` and you haven't already, please feel free to push a change to this
PR---or, if it gets merged before you can, feel free to open another PR.

In addition, if you'd like some stickers to put on your laptop, I'd be happy to help get 
them in the mail for you.  Just send an email with your physical mailing address to
stickers@mlpack.org, and then one of the mlpack maintainers will put some stickers in
an envelope for you.  It may take a few weeks to get them, depending on your location. :+1:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.