-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-20631][PYTHON][ML] LogisticRegression._checkThresholdConsistency should use values not Params #17891
Conversation
Test build #76548 has finished for PR 17891 at commit
|
Test build #76549 has finished for PR 17891 at commit
|
cc @jkbradley |
LGTM, merged into master and branch-2.2/2.1/2.0. Thanks! |
…cy should use values not Params ## What changes were proposed in this pull request? - Replace `getParam` calls with `getOrDefault` calls. - Fix exception message to avoid unintended `TypeError`. - Add unit tests ## How was this patch tested? New unit tests. Author: zero323 <zero323@users.noreply.github.com> Closes #17891 from zero323/SPARK-20631. (cherry picked from commit 804949c) Signed-off-by: Yanbo Liang <ybliang8@gmail.com>
…cy should use values not Params ## What changes were proposed in this pull request? - Replace `getParam` calls with `getOrDefault` calls. - Fix exception message to avoid unintended `TypeError`. - Add unit tests ## How was this patch tested? New unit tests. Author: zero323 <zero323@users.noreply.github.com> Closes #17891 from zero323/SPARK-20631. (cherry picked from commit 804949c) Signed-off-by: Yanbo Liang <ybliang8@gmail.com>
…cy should use values not Params ## What changes were proposed in this pull request? - Replace `getParam` calls with `getOrDefault` calls. - Fix exception message to avoid unintended `TypeError`. - Add unit tests ## How was this patch tested? New unit tests. Author: zero323 <zero323@users.noreply.github.com> Closes #17891 from zero323/SPARK-20631. (cherry picked from commit 804949c) Signed-off-by: Yanbo Liang <ybliang8@gmail.com>
Thanks @yanboliang! |
def logistic_regression_check_thresholds(self): | ||
self.assertIsInstance( | ||
LogisticRegression(threshold=0.5, thresholds=[0.5, 0.5]), | ||
LogisticRegressionModel |
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.
@zero323 @yanboliang Am I missing something? Why should the LogisticRegression constructor return an instance of type LogisticRegressionModel? Should you have to call fit?
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.
It shouldn't, but it's weird that Jenkins always passed the test. Anyway, let's fix it. Thanks for catching this.
…cy should use values not Params ## What changes were proposed in this pull request? - Replace `getParam` calls with `getOrDefault` calls. - Fix exception message to avoid unintended `TypeError`. - Add unit tests ## How was this patch tested? New unit tests. Author: zero323 <zero323@users.noreply.github.com> Closes apache#17891 from zero323/SPARK-20631.
@jkbradley It shouldn't. It is not a correct test #18085 |
…cy should use values not Params - Replace `getParam` calls with `getOrDefault` calls. - Fix exception message to avoid unintended `TypeError`. - Add unit tests New unit tests. Author: zero323 <zero323@users.noreply.github.com> Closes apache#17891 from zero323/SPARK-20631. (cherry picked from commit 804949c) Signed-off-by: Yanbo Liang <ybliang8@gmail.com>
What changes were proposed in this pull request?
getParam
calls withgetOrDefault
calls.TypeError
.How was this patch tested?
New unit tests.