-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fix for Issue #3655: Update Linear Regression Sample in Python Bindings #224
Conversation
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:
Thank you again for your contributions! 👍 |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -991,7 +991,7 @@ | |||
"outputs": [ |
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.
Line #1. model = mlpack.LinearRegression(check_input_matrices=False, copy_all_inputs=False, lambda_=0.0, verbose=False)
Aren't these all the default parameters? If so I would omit them just for the sake of cleanliness. Users can find more details on options in the documentation, there's no need to add them all here (it could seem overwhelming).
Reply via ReviewNB
@@ -991,7 +991,7 @@ | |||
"outputs": [ |
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.
Line #31. print(f"Mean Absoulte Error: {np.mean(mae_score):.2f}")
I know this isn't what you intended to fix, but can you fix the typo on this line while you're at it? :)
Reply via ReviewNB
@@ -991,7 +991,7 @@ | |||
"outputs": [ |
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.
Line #1. model = mlpack.LinearRegression(check_input_matrices=False, copy_all_inputs=False, lambda_=0.1, verbose=False)
Same comments on this cell about default parameters (and also the 'absoulte' typo :)).
Reply via ReviewNB
Thanks @MarkFischinger for looking into this! The fixes look good to me, just some minor comments. 👍 |
Hi @rcurtin, |
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.
Thanks, the fixes are much appreciated!
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.
Second approval provided automatically after 24 hours. 👍
This PR addresses issue #3655 reported by tunglinwood. The issue came from changes in the Linear Regression, Ridge Regression and Bayesian Linear Regression python bindings, which you can see in the mlpack documentation. The sample code provided earlier does not function as expected under these new changes.
Modifications