[SPARK-18710][ML] Add offset in GLM by actuaryzhang · Pull Request #16699 · apache/spark

actuaryzhang · 2017-01-25T07:12:39Z

What changes were proposed in this pull request?

Add support for offset in GLM. This is useful for at least two reasons:

Account for exposure: e.g., when modeling the number of accidents, we may need to use miles driven as an offset to access factors on frequency.
Test incremental effects of new variables: we can use predictions from the existing model as offset and run a much smaller model on only new variables. This avoids re-estimating the large model with all variables (old + new) and can be very important for efficient large-scaled analysis.

How was this patch tested?

New test.

@yanboliang @srowen @felixcheung @sethah

SparkQA · 2017-01-25T07:29:09Z

Test build #71974 has finished for PR 16699 at commit a1f5695.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-25T08:04:16Z

Test build #71975 has finished for PR 16699 at commit d071b95.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

zhengruifeng · 2017-01-25T09:26:48Z

you should not modify sharedParams directly.
"// DO NOT MODIFY THIS FILE! It was generated by SharedParamsCodeGen."

And if there is no other algorithms inheriting hasoffset, I suggest that do not create this

imatiach-msft · 2017-01-25T17:09:15Z

can you please:
1.) add [MLLIB] to the title
2.) fix the MiMa tests
The MiMa tests error seems to be related to the interface change, probably you need to add an exclude to the file "project/MimaExcludes.scala":

[error] * abstract method getOffsetCol()java.lang.String in trait org.apache.spark.ml.param.shared.HasOffsetCol is inherited by class GeneralizedLinearRegressionBase in current version.
[error] filter with: ProblemFilters.excludeInheritedNewAbstractMethodProblem
[error] * abstract method offsetCol()org.apache.spark.ml.param.Param in trait org.apache.spark.ml.param.shared.HasOffsetCol is inherited by class GeneralizedLinearRegressionBase in current version.
[error] filter with: ProblemFilters.excludeInheritedNewAbstractMethodProblem
[error] * abstract synthetic method org$apache$spark$ml$param$shared$HasOffsetCol$setter$offsetCol_=(org.apache.spark.ml.param.Param)Unit in trait org.apache.spark.ml.param.shared.HasOffsetCol is inherited by class GeneralizedLinearRegressionBase in current version.
[error] filter with: ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("

SparkQA · 2017-01-25T17:29:31Z

Test build #71994 has finished for PR 16699 at commit d2afcb0.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-25T18:44:38Z

Test build #71995 has finished for PR 16699 at commit 9eca1a6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

actuaryzhang · 2017-01-25T19:21:01Z

@zhengruifeng @imatiach-msft
Thanks much for pointing out the issue due to the hasOffset trait. This is what caused the test to fail. I have moved it to the GLRBase class. Things are working now 👍

imatiach-msft · 2017-01-25T20:25:53Z

 import org.apache.spark.internal.Logging
 import org.apache.spark.ml.feature.Instance
 import org.apache.spark.ml.linalg._
+import org.apache.spark.ml.regression.GLRInstance


this is a bit strange - you are using the GLRInstance that you added to specifically GeneralizedLinearRegression.scala in the more generic optimizer IterativelyReweightedLeastSquares.scala. It doesn't seem right for this file to depend on anything in the regression directory, it should really be the other way around.
I'm wondering if either:
1.) We can move the GLRInstance to a more generic place
2.) OR add it to Instance.scala as a separate case class eg "OffsetInstance"
3.) OR keep the offset a separate value

imatiach-msft · 2017-01-25T20:38:45Z

- val eta = model.predict(instance.features)
+ val reweightFunc: (GLRInstance, WeightedLeastSquaresModel) => (Double, Double) = {
+ (instance: GLRInstance, model: WeightedLeastSquaresModel) => {
+ val eta = model.predict(instance.features) + instance.offset


minor suggestion - instead of doing:
eta = prediction + offset
mu = fitted(eta)
newLabel = eta - offset + stuff
maybe do:
eta = prediction
mu = fitted(eta + offset)
newLabel = eta + stuff

imatiach-msft · 2017-01-25T20:55:44Z

+ val eta = BLAS.dot(features, coefficients) + intercept
+ familyAndLink.fitted(eta)
+ } else {
+ throw new SparkException("Must supply offset value when offset is set.")


maybe a better error message might be:
Must supply offset to predict when offset column is specified

imatiach-msft · 2017-01-25T21:02:34Z

+ "column name. If this is not set or empty, we treat all instance offsets as 0.0")
+
+ /** @group getParam */
+ def getOffsetCol: String = $(offsetCol)


it looks like you will need to update the validateAndTransformSchema method below to validate these parameters - eg check if the column exists? (similar to what the base class does for features/label columns)

thanks for fixing!

ghost · 2017-01-25T21:03:43Z

@actuaryzhang added a couple comments, please take a look, thanks!

ghost · 2017-01-25T22:19:45Z

@imatiach-msft Thanks so much for your detailed review. Incredibly helpful. I've addressed all your comments in the new commit. Major changes are highlighted below:

Create OffsetInstance in Instance.scala and remove dependency of IWLS on regression.
Add param checking for offset.

Let me know if there is anything else that needs improvement.

ghost · 2017-01-25T23:35:45Z

Test build #72003 has finished for PR 16699 at commit d44974c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ghost · 2017-01-26T15:07:19Z

 val model = if (familyObj == Gaussian && linkObj == Identity) {
 // TODO: Make standardizeFeatures and standardizeLabel configurable.
+ val wlsInstances: RDD[Instance] = instances.map { instance =>
+ Instance(instance.label - instance.offset, instance.weight, instance.features)


going over all OffsetInstance and converting again to Instance seems like it would be expensive and it would increase memory usage. What do you think about the alternative of making OffsetInstance inherit from Instance - but then you would have to change Instance from a case class - and then just passing Instance to the fit below? Is there anything else we could do here?

another thing you can do, if you don't want to change the hierarchy, is to move the initialization of instances inside the if/else. Then, for weightedleastsquares, you can just create RDD[Instance], but for IRLS weighted instances.

the more I think about this code, it looks like moving the initialization of instances inside the if/else below and creating Instance in one case and OffsetInstance in the other would save some memory/time and wouldn't force you to mess with the Instance case class.

ghost · 2017-01-26T15:07:46Z

+ }
+
+ /** Converts to an [[Instance]] object by leaving out the offset. */
+ private[ml] def toInstance: Instance = Instance(label, weight, features)


it looks like this method is only used once in a test case, might it be better to remove it?

actually, another alternative might be to make OffsetInstance inherit from Instance (which I wrote below as well). What do you think about this idea?

Yes, this is only used once in the current code, and I can get rid of it. But I feel other regression-type models may use offset at some point and having this method will make it easier to switch between Instance and OffsetInstance.

ghost · 2017-01-26T15:18:07Z

 private lazy val familyAndLink = new FamilyAndLink(familyObj, linkObj)

 override protected def predict(features: Vector): Double = {
- val eta = predictLink(features)


it looks like if the family = Gaussian and link = Identity you don't even need to check for offsetCol here

ghost · 2017-01-26T15:29:55Z

- val predictLinkUDF = udf { (features: Vector) => predictLink(features) }
+ val predictUDF = udf { (features: Vector, offset: Double) => predict(features, offset) }
+ val predictLinkUDF = udf { (features: Vector, offset: Double) => predictLink(features, offset) }
+ val off = if (!isSet(offsetCol) || $(offsetCol).isEmpty) lit(0.0) else col($(offsetCol))


calling the offset column "off" was a little confusing to me, maybe we can give it a better name, like "offsetCol" or even "offset". When I looked at the code below I first thought it was some sort of flag passed to the predict function.

also, as I mentioned above, if family = Gaussian and link = Identity we shouldn't be passing any offset, but then the user shouldn't be setting an offset column probably - so either adding that to the validation logic or to the code here or to the predict function you call below would fix this unusual case; I think this seems like a validation issue and probably should be added to the validation method.

ghost · 2017-01-26T15:37:25Z

+ Vectors.dense(-1.9991044, 0.7247511, 0.1424392),
+ Vectors.dense(-0.27378146, 0.31599396, -0.06204946))
+
+ import GeneralizedLinearRegression._


should this be moved to the imports above?

ghost · 2017-01-26T15:39:21Z

+ var idx = 0
+ for (fitIntercept <- Seq(false, true)) {
+ for (family <- Seq("gaussian", "poisson", "gamma")) {
+ val trainer = new GeneralizedLinearRegression().setFamily(family)


it looks like the spacing is off a little here? the val trainer = ... is at the same level as the for above. I'm surprised the style checker didn't catch something like this.

ghost · 2017-06-27T21:49:48Z

@yanboliang Thanks much for the review. The new commit includes everything you suggested except implementing WeightLeastSquares interface for OffsetInstance. Please see my incline comments above.

ghost · 2017-06-27T22:29:42Z

Test build #78727 has finished for PR 16699 at commit 1e47a11.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

ghost · 2017-06-27T22:30:44Z

jenkins, retest this please

ghost · 2017-06-27T23:09:28Z

Test build #78729 has finished for PR 16699 at commit 1e47a11.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

ghost · 2017-06-27T23:19:56Z

Not sure what this error msg means, but it seems unrelated to this PR.

ghost · 2017-06-28T05:01:31Z

this is a known issue in test runs currently - it's mentioned in dev@spark.apache.org, just so you know.

ghost · 2017-06-28T06:00:29Z

Got it. I should pay more attention to that mailing list from now on :)

ghost · 2017-06-28T14:58:04Z

Jenkins, test this please.

ghost · 2017-06-28T16:02:19Z

Test build #78812 has finished for PR 16699 at commit 1e47a11.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ghost

LGTM after resolving some trivial issues. Thanks for contributing this great feature.

ghost · 2017-06-29T07:53:12Z

+ features: Vector) {
+
+ /** Constructs from an [[Instance]] object and offset */
+ def this(instance: Instance, offset: Double = 0.0) = {


Remove it if it was not used anymore.

ghost · 2017-06-29T07:54:06Z

+ }
+
+ /** Converts to an [[Instance]] object by leaving out the offset. */
+ private[ml] def toInstance: Instance = Instance(label, weight, features)


Remove private[ml] since you have marked the whole class as private[ml].

ghost · 2017-06-29T08:13:29Z

+ * as 0.0. The feature specified as offset has a constant coefficient of 1.0.
+ * @group param
+ */
+ final val offsetCol: Param[String] = new Param[String](this, "offsetCol", "The offset " +


@Since("2.3.0")

ghost · 2017-06-29T08:13:43Z

+ "column name. If this is not set or empty, we treat all instance offsets as 0.0")
+
+ /** @group getParam */
+ def getOffsetCol: String = $(offsetCol)


@Since("2.3.0")

ghost · 2017-06-29T08:14:43Z

+ * Default is not set, so all instances have offset 0.0.
+ *
+ * @group setParam
+ */


2.2.0 -> 2.3.0

ghost · 2017-06-29T08:28:01Z

+ /**
+ * Calculates the predicted value when offset is set.
+ */
+ def predict(features: Vector, offset: Double): Double = {


Mark it private, since we don't support predict on single instance for all models currently.

ghost · 2017-06-29T09:32:13Z

@@ -961,14 +1008,16 @@ class GeneralizedLinearRegressionModel private[ml] (
 }

 override protected def transformImpl(dataset: Dataset[_]): DataFrame = {


I summarized all four cases for making prediction as following:
Estimator(training data) Transformer(prediction data) How R predict How Spark predict
w/ offset column w/ offset column use offset of prediction data use offset of prediction data
w/ offset column w/o offset column use offset of training data not use offset
w/o offset column w/ offset column not use offset not use offset
w/o offset column w/o offset column not use offset not use offset
For case 1 and 4, there is not that controversial.
For case 2, the reason behind a different way to handle is we can't store all offset data in our model like what R does, but we should print a warning log to let users know that is different from R.
For case 3, in your current implementation, it ignores whether the model was trained with offset. I think it might be worth discussing. I think the correct way should consider whether the model was trained with offset. If the model was trained without offset, we should ignore the offset column when making prediction on new dataset. Or at least, we should print out warning to remind users.
However, I think we can discuss and resolve this issue in follow-up work. @actuaryzhang What do you think my proposal of how Spark make prediction? Thanks.

Thanks for summarizing the different cases. I think this is worth a deeper discussion as follow-up work. Let me work on this in another PR.

ghost · 2017-06-29T17:51:03Z

Made a new commit that fixes the issues you pointed out.

ghost · 2017-06-29T18:51:36Z

Test build #78920 has finished for PR 16699 at commit db0ac93.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ghost · 2017-06-30T12:01:24Z

Merged into master. Thanks for the contribution and all reviews! This great feature will benefit lots of users.
@actuaryzhang Could you send follow-up PRs to address the two inline comments? Thanks.

ghost · 2017-06-30T12:27:47Z

@yanboliang @actuaryzhang this PR breaks the scala-2.10 build:

[error] /home/jenkins/workspace/spark-master-compile-maven-scala-2.10/mllib/src/test/scala/org/apache/spark/ml/regression/GeneralizedLinearRegressionSuite.scala:1117: value absTol is not a member of Int
[error] assert(summary.dispersion ~== dispersionR absTol 1E-3)
[error] ^
[error] /home/jenkins/workspace/spark-master-compile-maven-scala-2.10/mllib/src/test/scala/org/apache/spark/ml/regression/GeneralizedLinearRegressionSuite.scala:1232: value absTol is not a member of Int

See: https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Compile/job/spark-master-compile-maven-scala-2.10/4721/

ghost · 2017-06-30T14:12:12Z

@hvanhovell I will send a quick fix soon, thanks for your kindly remind.

ghost · 2017-06-30T16:06:30Z

#18489 fixed the build failure. Thanks.

actuaryzhang added 5 commits January 24, 2017 15:46

add trait offset
3bf2718

add offset setter
0e240eb

implement offset in GLM
9c41453

add test for glm with offset
7823f8a

minor cleanup
a1f5695

add doc for GLRInstance
d071b95

actuaryzhang changed the title ~~[SPARK-18710] Add offset in GLM~~[SPARK-18710][ML] Add offset in GLMJan 25, 2017

remove offset from shared param
d2afcb0

fix style issue
9eca1a6

imatiach-msft reviewed Jan 25, 2017
View reviewed changes

rename to OffsetInstance and add param check
d44974c

imatiach-msft reviewed Jan 26, 2017
View reviewed changes

address comments
1e47a11

yanboliang reviewed Jun 29, 2017
View reviewed changes

address comments
db0ac93

asfgit closed this in 49d767dJun 30, 2017

actuaryzhang deleted the offset branch June 30, 2017 15:41

actuaryzhang mentioned this pull request Jul 1, 2017
[SPARK-21275][ML] Update GLM test to use supportedFamilyNames #18495
Closed

actuaryzhang mentioned this pull request Jul 5, 2017
[SPARK-21310][ML][PySpark] Expose offset in PySpark #18534
Closed

actuaryzhang mentioned this pull request Aug 3, 2017
[SPARK-21622][ML][SparkR] Support offset in SparkR GLM #18831
Closed

		@@ -961,14 +1008,16 @@ class GeneralizedLinearRegressionModel private[ml] (
		}

		override protected def transformImpl(dataset: Dataset[_]): DataFrame = {

Estimator(training data)	Transformer(prediction data)	How R predict	How Spark predict
w/ offset column	w/ offset column	use offset of prediction data	use offset of prediction data
w/ offset column	w/o offset column	use offset of training data	not use offset
w/o offset column	w/ offset column	not use offset	not use offset
w/o offset column	w/o offset column	not use offset	not use offset

Uh oh!

Conversation

actuaryzhang commented Jan 25, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jan 25, 2017

Uh oh!

SparkQA commented Jan 25, 2017

Uh oh!

zhengruifeng commented Jan 25, 2017

Uh oh!

imatiach-msft commented Jan 25, 2017

Uh oh!

SparkQA commented Jan 25, 2017

Uh oh!

SparkQA commented Jan 25, 2017

Uh oh!

actuaryzhang commented Jan 25, 2017

Uh oh!

imatiach-msftJan 25, 2017

Choose a reason for hiding this comment

Uh oh!

imatiach-msftJan 25, 2017

Choose a reason for hiding this comment

Uh oh!

imatiach-msftJan 25, 2017

Choose a reason for hiding this comment

Uh oh!

imatiach-msftJan 25, 2017

Choose a reason for hiding this comment

Uh oh!

imatiach-msftJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 25, 2017

Uh oh!

ghost commented Jan 25, 2017

Uh oh!

ghost commented Jan 25, 2017

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghostJan 26, 2017

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 27, 2017

Uh oh!

ghost commented Jun 27, 2017

Uh oh!

ghost commented Jun 27, 2017

Uh oh!

ghost commented Jun 30, 2017•
edited by yanboliang
Loading