[SPARK-10479] [SPARK-10480] [ML] Fix ML.LinearRegressionModel.copy() #8641
Conversation
|
Looks OK. On the borderline of needing a JIRA. really this is a fix for SPARK-8538 / SPARK-8539 I think. CC @feynmanliang |
|
Test build #42093 has finished for PR 8641 at commit
|
|
LGTM, just noticed that LogisticRegression doesn't copy the summary object, SPARK-10479 will track that work |
yanboliang
changed the title
|
@yanboliang how about fixing SPARK-10479 here too? it's very similar. |
|
@srowen OK, I have updated this PR to fix both SPARK-10479 and SPARK-10480 |
| @@ -468,7 +468,9 @@ class LogisticRegressionModel private[ml] ( | |||
| } | |||
|
|
|||
| override def copy(extra: ParamMap): LogisticRegressionModel = { | |||
| copyValues(new LogisticRegressionModel(uid, weights, intercept), extra).setParent(parent) | |||
| val newModel = copyValues(new LogisticRegressionModel(uid, weights, intercept), extra) | |||
| if (trainingSummary.isDefined) newModel.setSummary(trainingSummary.get) | |||
There was a problem hiding this comment.
Just checking, was this intended as well? does it need to happen for LinearRegression too? LGTM otherwise.
There was a problem hiding this comment.
Yes, it's needed. Because after we new LogisticRegressionModel or LinearRegressionModel, the default value of trainingSummary is None. So we should also copy trainingSummary explicitly to the new model.
There was a problem hiding this comment.
nit. newModel.trainingSummary = trainingSummary should work the same.
|
Test build #42130 has finished for PR 8641 at commit
|
|
Please add [SPARK-10479] to PR title |
|
LGTM |
|
Merged into master and branch-1.5. Thanks! |
yanboliang
changed the title
yanboliang
changed the title
This PR fix two model
copy()related issues:SPARK-10480
ML.LinearRegressionModel.copy()ignored argumentextra, it will not take effect when users setting this parameter.SPARK-10479
ML.LogisticRegressionModel.copy()should copy model summary if available.