Added checks for relative input shapes in linear regression using the utility introduced in #2370 #3640
Conversation
…tatements to check input shapes
|
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! 👍 |
| @@ -163,7 +163,7 @@ void BINDING_FUNCTION(util::Params& params, util::Timers& timer) | |||
| timer.Start("load_responses"); | |||
| responses = params.Get<rowvec>("training_responses"); | |||
| timer.Stop("load_responses"); | |||
|
|
|||
There was a problem hiding this comment.
No need to add the whitespace (sorry, pedantic comment).
| "training file." << std::endl; | ||
| } | ||
| util::CheckSameDimensionality(predictors, parameters.n_rows - 1, | ||
| "LinearRegression::ComputeError()", "predictors"); |
There was a problem hiding this comment.
I believe we can remove the nRows variable now; it appears to be unused.
There was a problem hiding this comment.
Sure I'll remove the nRows declaration.
| try | ||
| { | ||
| util::CheckSameDimensionality(points, lr->Parameters().n_elem - 1, | ||
| "Linear Regression Prediction", "test points"); |
There was a problem hiding this comment.
I'm not sure it's even necessary to keep this check---Predict() already has a CheckSameDimensionality() call inside it. So I think we can remove this (and the same for linear_regression_predict_main.cpp).
There was a problem hiding this comment.
the build tests failed upon removing this check I reverted this change and made a commit.
There was a problem hiding this comment.
What did you try to debug the issue? "The build tests failed" is not a sufficient reason to give up on the comment here.
|
@rcurtin I have made all the changes mentioned in the comments. The code is passing all the build tests (except windows). If there are any more changes do let me know. I'll move on to kmeans after this one is merged. |
lumi232
force-pushed
the
linear_regression_added_shape_utility
branch
from
40676a7
to
9ecf0aa
Compare
|
@rcurtin can you review this PR. |
|
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍 |
| @@ -163,7 +163,6 @@ void BINDING_FUNCTION(util::Params& params, util::Timers& timer) | |||
| timer.Start("load_responses"); | |||
| responses = params.Get<rowvec>("training_responses"); | |||
| timer.Stop("load_responses"); | |||
|
|
|||
There was a problem hiding this comment.
Sorry, what I meant was that there was no need to replace the blank line with a line that has four spaces. I didn't mean that we should remove the line. 👍
| try | ||
| { | ||
| util::CheckSameDimensionality(points, lr->Parameters().n_elem - 1, | ||
| "Linear Regression Prediction", "test points"); |
There was a problem hiding this comment.
What did you try to debug the issue? "The build tests failed" is not a sufficient reason to give up on the comment here.
|
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍 |
This PR implements the solution mentioned in the issue #2820 by using the CheckSameSizes and CheckSameDimensionality utilities in src/mlpack/methods/linear_regression/linear_regression_train_main.cpp
src/mlpack/methods/linear_regression/linear_regression_main.cpp and
src/mlpack/methods/linear_regression/linear_regression_predict_main.cpp
All the if statements mentioned in #2370 (review) were replaced with the utility functions.
I plan to implement the solution to other methods as well once this PR is merged successfully. This is my first PR I look forward to contribute more to the organisation. Looking forward for the review.