[SPARK-11852] [ML] StandardScaler minor refactor #9839
Conversation
| def checkModelData(model1: StandardScalerModel, model2: StandardScalerModel): Unit = { | ||
| assert(model1.mean === model2.mean) | ||
| assert(model1.std === model2.std) | ||
| } |
There was a problem hiding this comment.
We only need to check mean and std which are parts of the model, withStd and withStd are params.
| ) | ||
| val df = sqlContext.createDataFrame(data.zip(resWithBoth)).toDF("features", "expected") | ||
| val standardScaler = new StandardScaler() | ||
| testEstimatorAndModelReadWrite(standardScaler, df, allParams, checkModelData) | ||
| } |
There was a problem hiding this comment.
withStd and withStd of StandardScalerModel must be inherited from StandardScaler, so we can not construct StandardScalerModel directly by specifying the two variables. Here we combine the original test cases into one with testEstimatorAndModelReadWrite which both test the estimator and model.
There was a problem hiding this comment.
I think this is not an ideal unit test for read/write because the model fitting part shouldn't be part of it, which is already covered by other tests. Constructing estimator and model directly can save some test time.
|
Test build #46330 has finished for PR 9839 at commit
|
|
Test build #46333 has finished for PR 9839 at commit
|
| @@ -189,7 +189,6 @@ object StandardScalerModel extends MLReadable[StandardScalerModel] { | |||
| sqlContext.read.parquet(dataPath) | |||
| .select("std", "mean", "withStd", "withMean") | |||
| .head() | |||
| // This is very likely to change in the future because withStd and withMean should be params. | |||
There was a problem hiding this comment.
If withMean and withStd are parameters, we should save them in metadata/ but not both under data/ and medadata/. Can we change the constructor of ml.StandardScalerModel to take only std and mean but construct scaler only inside transform? So scaler is no longer a member variable. We can fix performance issues in 1.7.
|
Jenkins, test this please. |
|
Test build #46413 has finished for PR 9839 at commit
|
|
Test build #46415 has finished for PR 9839 at commit
|
|
LGTM. Merged into master and branch-1.6. Thanks! |
withStdandwithMeanshould be params ofStandardScalerandStandardScalerModel.