Skip to content

Conversation

@huaxingao
Copy link
Contributor

What changes were proposed in this pull request?

Add training summary for LinearSVCModel......

Why are the changes needed?

so that user can get the training process status, such as loss value of each iteration and total iteration number.

Does this PR introduce any user-facing change?

Yes
LinearSVCModel.summary
LinearSVCModel.evaluate

How was this patch tested?

new tests

@SparkQA
Copy link

SparkQA commented Jun 21, 2020

Test build #124333 has finished for PR 28884 at commit a30312d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • sealed trait LinearSVCSummary extends BinaryClassificationSummary
  • sealed trait LinearSVCTrainingSummary extends LinearSVCSummary with TrainingSummary
  • class LinearSVCModel(_JavaClassificationModel, _LinearSVCParams, JavaMLWritable, JavaMLReadable,
  • class LinearSVCSummary(_BinaryClassificationSummary):
  • class LinearSVCTrainingSummary(LinearSVCSummary, _TrainingSummary):

Copy link
Contributor

@zhengruifeng zhengruifeng left a comment

Choose a reason for hiding this comment

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

In general LGTM
this is the first case to use rawPrediction to compute aur/prc in summaries.

* the current model
*/
private[classification] def findSummaryModel():
(LinearSVCModel, String, String) = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
private[classification] def findSummaryModel(): (LinearSVCModel, String, String) = { ?

/**
* Abstraction for LinearSVC results for a given model.
*/
sealed trait LinearSVCSummary extends BinaryClassificationSummary {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: uncessary braces

* Abstraction for LinearSVC training results.
*/
sealed trait LinearSVCTrainingSummary extends LinearSVCSummary with TrainingSummary {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

val intercept = if ($(fitIntercept)) rawCoefficients.last else 0.0
copyValues(new LinearSVCModel(uid, Vectors.dense(coefficientArray), intercept))
val vec = Vectors.dense(coefficientArray)
return createModel(dataset, vec, intercept, objectiveHistory)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like: createModel(dataset, Vectors.dense(coefficientArray), intercept, objectiveHistory)
var vec and return are unnecessary

@SparkQA
Copy link

SparkQA commented Jun 23, 2020

Test build #124386 has finished for PR 28884 at commit 423eeb5.

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

@huaxingao
Copy link
Contributor Author

cc @srowen

@srowen
Copy link
Member

srowen commented Jun 24, 2020

Looking good. There's really no downside here right? just works like the other summaries, and is adding this for consistency.

@huaxingao
Copy link
Contributor Author

Right, it works like the other summaries. I don't see any downside for this.

@srowen srowen closed this in 8795133 Jun 26, 2020
@huaxingao
Copy link
Contributor Author

Thanks! @srowen @zhengruifeng

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants