[SPARK-24283][ML] Make ml.StandardScaler skip conversion of Spar… #21942
Conversation
…k ml vectors to mllib vectors
|
@sujithjay, thanks! I am a bot who has found some folks who might be able to help with the review:@mengxr, @jkbradley and @MLnick |
|
@holdenk Could you also please take a look at this PR? |
hhbyyh
left a comment
hhbyyh
left a comment
There was a problem hiding this comment.
IMO it's a good thing that we move the implementation from MLlib to ML. Yet I do see one concern about test coverage. Most of the unit tests are still in MLlib and ML does not provide equivalent converge, which makes it difficult to verify code changes.
One solution is to move all the unit tests to ML too, though this will double the unit test time.
| * for the column with zero std. | ||
| */ | ||
| @Since("2.3.0") | ||
| def transform(vector: Vector): Vector = { |
Change access modifier of `transform` method from public to private[spark]. Also, remove `@Since` annotation from the method.
|
@hhbyyh I, personally, like the idea of moving the unit tests to ML. However, are you suggesting I move some of them as part of this PR? Or were you suggesting that it is a task we need to take up separately? |
|
I think it's better to move the code and unit test in one PR. But since it's not a trivial change, I suggest you to wait for committers' opinion. |
|
Hi @mengxr, @jkbradley, @MLnick and @holdenk , could you please review this PR ? Thank you. |
|
Hi @mengxr, @jkbradley, @MLnick, @holdenk, @viirya , could you please review this PR ? |
|
Sorry I've been slow to respond, I'll take a look today. |
holdenk
left a comment
holdenk
left a comment
There was a problem hiding this comment.
Thanks for working to get us off of MLlib :) I'm +1 on the need to move the tests over, it will temporarily increase our total test time but that's better than not having coverage. If you wouldn't mind a quick micro-benchmark showing that it's not a performance regression over the old code-path would be great as well.
| * `lazy val` so it will be evaluated in that branch. Note that we don't | ||
| * want to create this array multiple times in `transform` function. | ||
| */ | ||
| private lazy val shift: Array[Double] = mean.toArray |
There was a problem hiding this comment.
How does this interplay with serialization? Would it make sense to evaluate this before we serialize the UDF so it isn't done on each worker or?
| } | ||
| } | ||
| Vectors.dense(values) | ||
| } else if ($(withStd)) { |
There was a problem hiding this comment.
Maybe leave a comment withStd and not mean since when tracing the code by hand the nested if/else if can get a bit confusing flow wise.
|
@sujithjay are you still active on this PR? Or shall someone take this over? |
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
@zhengruifeng are you working on something similar or would you care to review?
|
@srowen After the crrently merged pr Make the transform natively in ml framework to avoid extra conversion, And in RobustScaler, I am to make the transformation more common, so that |
8 participants
…k ml vectors to mllib vectors
What changes were proposed in this pull request?
Currently, ml.StandardScaler converts Spark ml vectors to mllib vectors during transform operation. This PR makes changes to skip this step.
How was this patch tested?
Existing tests in StandardScalerSuite