Skip to content

[SPARK-4581][MLlib] Refactorize StandardScaler to improve the transformation performance #3435

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.

Sign up for GitHub

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

Closed
dbtsai wants to merge 6 commits into from
Closed

[SPARK-4581][MLlib] Refactorize StandardScaler to improve the transformation performance #3435

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5bffd3d
first commit
Nov 24, 2014
fc795e4
update
Nov 25, 2014
9c51eef
style
Nov 25, 2014
cdb5cef
small change
Nov 25, 2014
daf2b06
Address the feedback
Nov 25, 2014
85885a9
revert to have lazy in shift array.
Nov 25, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
update
  • Loading branch information
DB Tsai committed Nov 25, 2014
commit fc795e4d7f8792ce4ad70b7ecb41531556669983
3 changes: 2 additions & 1 deletion mllib/src/main/scala/org/apache/spark/mllib/feature/StandardScaler.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,14 +96,14 @@ class StandardScalerModel private[mllib] (
*/
override def transform(vector: Vector): Vector = {
require(mean.size == vector.size)
val localFactor = factor
if (withMean) {
val localShift = shift
Copy link
Contributor

@mengxr mengxr Nov 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worth to leave a comment here and explain why we need local reference.

Copy link
Contributor

@mengxr mengxr Nov 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shift is only used in this branch. Shall we just put val shift = mean.toArray here instead of having a member variable?

Copy link
Member Author

@dbtsai dbtsai Nov 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I'll change it back to lazy since it will not be evaluated in those branches which don't use shift. I don't want to create shift array/object for each sample since shift will always be the same.

Copy link
Contributor

@mengxr mengxr Nov 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shift only holds a reference to mean.values. We don't really need to define it as a member and make it lazy. It should give the same performance if we only define it inside the if branch.

Copy link
Member Author

@dbtsai dbtsai Nov 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For different implementation of vector, toArray can be very expensive. For example, toArray for sparse vector requires to create a new array object and loop through all the non zero values. As a result, we can have a global lazy shift which can prevent this happens.

vector match {
case dv: DenseVector =>
val values = dv.values.clone()
var i = 0
if(withStd) {
val localFactor = factor
while (i < values.length) {
values(i) = (values(i) - localShift(i)) * localFactor(i)
i += 1
Expand All @@ -118,6 +118,7 @@ class StandardScalerModel private[mllib] (
case v => throw new IllegalArgumentException("Do not support vector type " + v.getClass)
}
} else if (withStd) {
val localFactor = factor
vector match {
case dv: DenseVector =>
val values = dv.values.clone()
Expand Down