[SPARK-20351] [ML] Add trait hasTrainingSummary to replace the duplicate code by hhbyyh · Pull Request #17654 · apache/spark

hhbyyh · 2017-04-16T23:44:31Z

What changes were proposed in this pull request?

Add a trait HasTrainingSummary to avoid code duplicate related to training summary.

Currently all the training summary use the similar pattern which can be generalized,


private[ml] final var trainingSummary: Option[T] = None
def hasSummary: Boolean = trainingSummary.isDefined
def summary: T = trainingSummary.getOrElse...
private[ml] def setSummary(summary: Option[T]): ...

Classes with the trait need to override setSummry. And for Java compatibility, they will also have to override summary method, otherwise the java code will regard all the summary class as Object due to a known issue with Scala.

How was this patch tested?

existing Java and Scala unit tests

SparkQA · 2017-04-17T00:44:34Z

Test build #75845 has finished for PR 17654 at commit 3bca3b1.

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

SparkQA · 2018-09-18T21:38:55Z

Test build #96194 has finished for PR 17654 at commit 3bca3b1.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

holdenk · 2018-09-27T14:19:38Z

Thanks for working on this, remove duplicated code is great. I'm curious as to why we couldn't remove some of the function calls to super and instead depend on inheritance?

If it's the types on the setters could we add another type parameter of the model?

srowen · 2018-11-21T23:08:51Z

 extends RegressionModel[Vector, GeneralizedLinearRegressionModel]
- with GeneralizedLinearRegressionBase with MLWritable {
+ with GeneralizedLinearRegressionBase with MLWritable
+ with HasTrainingSummary[GeneralizedLinearRegressionTrainingSummary]{


Nit: space before braces

Updated. Thanks

It looks like there still isn't a space here.

srowen · 2018-11-21T23:09:35Z

 override val numFeatures: Int = coefficients.size

+ private[ml]
+ override def setSummary(summary: Option[LinearRegressionTrainingSummary]): this.type =


Yeah, why do we need these definitions that just invoke the superclass method?

srowen · 2018-11-21T23:10:04Z

+ *
+ * @tparam T Summary instance type
+ */
+@Since("2.3.0")


Let's target 3.0.0 at this point.

holdenk · 2018-12-07T17:18:44Z

Gentle ping here, it's out of sync with master if you've got the time to bring it up to date that would be great.

hhbyyh · 2018-12-13T01:20:28Z

Ah, just see this. Will update it now.

SparkQA · 2018-12-14T09:21:15Z

Test build #100138 has finished for PR 17654 at commit ecef1d0.

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

srowen

Looking fine except for a few nits and a question about whether to keep the overrides.

srowen · 2018-12-14T13:55:37Z

- /** Indicates whether a training summary exists for this model instance. */
- @Since("1.5.0")
- def hasSummary: Boolean = trainingSummary.isDefined
+ override def summary: LinearRegressionTrainingSummary = super.summary


On the one hand, you don't need these overrides for this to work correctly, right? but I suppose it's necessary to preserve the @Since tag, which varies across implementations. But these were mostly introduced in 1.5.0, and where they have a later @Since tag, it matches when the class was introduced. I think it would also be coherent, for Spark 3.0, to remove these overrides, and mark the methods in the new trait as @Since 1.5.0. The result would be similar to what would happen if this had been introduced at the start. I don't feel strongly about it but what do you think? would clean up the code a little more.

I got an error message from Java side when removing summary
/home/yuhao/workspace/github/hhbyyh/spark/mllib/src/test/java/org/apache/spark/ml/classification/JavaLogisticRegressionSuite.java:145: error: incompatible types: Object cannot be converted to LogisticRegressionTrainingSummary [error] LogisticRegressionTrainingSummary summary = model.summary();

Ah OK nevermind then. Thanks for checking.

srowen · 2018-12-14T13:56:17Z

+
+ /**
+ * Gets summary of model on training set. An exception is
+ * thrown if `trainingSummary == None`.


Nit: from the callers perspective they don't know what trainingSummary is. "if hasSummary is false"?

One more nit @hhbyyh - can we change this one too?

Sure. Thanks for checking.

srowen · 2018-12-14T13:56:47Z

 extends RegressionModel[Vector, GeneralizedLinearRegressionModel]
- with GeneralizedLinearRegressionBase with MLWritable {
+ with GeneralizedLinearRegressionBase with MLWritable
+ with HasTrainingSummary[GeneralizedLinearRegressionTrainingSummary]{


It looks like there still isn't a space here.

SparkQA · 2018-12-15T03:24:31Z

Test build #100172 has finished for PR 17654 at commit 79ae7fe.

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

SparkQA · 2018-12-17T02:16:04Z

Test build #100210 has finished for PR 17654 at commit 11ba5f6.

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

srowen · 2018-12-17T15:28:28Z

Merged to master

hhbyyh · 2018-12-17T16:44:02Z

Thanks for the review. @srowen

…te code ## What changes were proposed in this pull request? Add a trait HasTrainingSummary to avoid code duplicate related to training summary. Currently all the training summary use the similar pattern which can be generalized, ``` private[ml] final var trainingSummary: Option[T] = None def hasSummary: Boolean = trainingSummary.isDefined def summary: T = trainingSummary.getOrElse... private[ml] def setSummary(summary: Option[T]): ... ``` Classes with the trait need to override `setSummry`. And for Java compatibility, they will also have to override `summary` method, otherwise the java code will regard all the summary class as Object due to a known issue with Scala. ## How was this patch tested? existing Java and Scala unit tests Closesapache#17654 from hhbyyh/hassummary. Authored-by: Yuhao Yang <yuhao.yang@intel.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

…e in PySpark ## What changes were proposed in this pull request? Python version of apache#17654 ## How was this patch tested? Existing Python unit test Closesapache#23676 from huaxingao/spark26754. Authored-by: Huaxin Gao <huaxing@us.ibm.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

…te code ## What changes were proposed in this pull request? Add a trait HasTrainingSummary to avoid code duplicate related to training summary. Currently all the training summary use the similar pattern which can be generalized, ``` private[ml] final var trainingSummary: Option[T] = None def hasSummary: Boolean = trainingSummary.isDefined def summary: T = trainingSummary.getOrElse... private[ml] def setSummary(summary: Option[T]): ... ``` Classes with the trait need to override `setSummry`. And for Java compatibility, they will also have to override `summary` method, otherwise the java code will regard all the summary class as Object due to a known issue with Scala. ## How was this patch tested? existing Java and Scala unit tests Closesapache#17654 from hhbyyh/hassummary. Authored-by: Yuhao Yang <yuhao.yang@intel.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

…e in PySpark ## What changes were proposed in this pull request? Python version of apache#17654 ## How was this patch tested? Existing Python unit test Closesapache#23676 from huaxingao/spark26754. Authored-by: Huaxin Gao <huaxing@us.ibm.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

has summary trait
3bca3b1

hhbyyh mentioned this pull request Jul 5, 2017
[SPARK-18724][ML] Add TuningSummary for TrainValidationSplit and CrossValidator #16158
Closed

srowen reviewed Nov 21, 2018
View reviewed changes

YY-OnCall added 4 commits December 12, 2018 23:19

resolve conflict
ffbdb6e

Merge remote-tracking branch 'upstream/master' into hassummary
4b660e3

Merge remote-tracking branch 'upstream/master' into hassummary
0008b89

remove unnecessary setter
ecef1d0

srowen reviewed Dec 14, 2018
View reviewed changes

fix comment and style
79ae7fe

srowen approved these changes Dec 15, 2018
View reviewed changes

update comment
11ba5f6

srowen closed this in c04ad17Dec 17, 2018

huaxingao mentioned this pull request Jan 28, 2019
[SPARK-26754][PYTHON] Add hasTrainingSummary to replace duplicate code in PySpark #23676
Closed

Uh oh!

Conversation

hhbyyh commented Apr 16, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 17, 2017

Uh oh!

SparkQA commented Sep 18, 2018

Uh oh!

holdenk commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holdenk commented Dec 7, 2018

Uh oh!

hhbyyh commented Dec 13, 2018

Uh oh!

SparkQA commented Dec 14, 2018

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 15, 2018

Uh oh!

SparkQA commented Dec 17, 2018

Uh oh!

srowen commented Dec 17, 2018

Uh oh!

hhbyyh commented Dec 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hhbyyh commented Apr 16, 2017•
edited
Loading