[SPARK-35678][ML][FOLLOWUP] softmax support offset and step #32927

zhengruifeng · 2021-06-16T08:17:03Z

What changes were proposed in this pull request?

use newly impled softmax function in NB

Why are the changes needed?

to simplify impl

Does this PR introduce any user-facing change?

No

How was this patch tested?

existing testsuite

SparkQA · 2021-06-16T09:31:24Z

Test build #139863 has finished for PR 32927 at commit cbcb584.

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

SparkQA · 2021-06-16T11:54:32Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44392/

SparkQA · 2021-06-16T12:29:47Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44392/

srowen

This is fine, but are there other places it can be used? let's get as many as we can in one go in one PR / JIRA if possible. They're logically related

zhengruifeng · 2021-06-16T15:17:50Z

@srowen Sorry for missing this one.
There are other softmax computations in ML (ANN), but seems need to make softmax support offset & step, I will have a try.

zhengruifeng · 2021-06-16T16:03:18Z

...src/main/scala/org/apache/spark/ml/optim/aggregator/MultinomialLogisticBlockAggregator.scala

    var localLossSum = 0.0
    var localWeightSum = 0.0
    var i = 0
-    val probs = Array.ofDim[Double](numClasses)


when softmax support offset & step, this tmp array is no longer needed

zhengruifeng · 2021-06-16T16:03:51Z

...src/main/scala/org/apache/spark/ml/optim/aggregator/MultinomialLogisticBlockAggregator.scala

    var localWeightSum = 0.0
    var i = 0
-    val probs = Array.ofDim[Double](numClasses)
-    val multiplierSum = Array.ofDim[Double](numClasses)


multiplierSum is only needed when fitIntercept is true

zhengruifeng · 2021-06-16T16:04:39Z

...src/main/scala/org/apache/spark/ml/optim/aggregator/MultinomialLogisticBlockAggregator.scala

-  private val marginOffset = if (fitWithMean) {
-    val offset = intercept.copy
+  @transient private lazy val marginOffset = if (fitWithMean) {
+    val offset = new DenseVector(coefficientsArray.takeRight(numClasses)) // intercept


do not call intercept to avoid trigger the lazy initialization of intercept

zhengruifeng · 2021-06-16T16:05:16Z

mllib/src/main/scala/org/apache/spark/ml/ann/LossFunction.scala

  val weights = new BDV[Double](0)

  override def eval(data: BDM[Double], output: BDM[Double]): Unit = {
+    require(!data.isTranspose && !output.isTranspose)


The two BDM are expected to be not transposed here

SparkQA · 2021-06-16T16:57:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44409/

SparkQA · 2021-06-16T17:08:13Z

Test build #139879 has finished for PR 32927 at commit 6ee9054.

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

SparkQA · 2021-06-16T17:32:39Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44409/

zhengruifeng · 2021-06-17T07:08:12Z

retest this please

SparkQA · 2021-06-17T08:05:42Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44435/

SparkQA · 2021-06-17T08:24:31Z

Test build #139905 has finished for PR 32927 at commit 6ee9054.

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

SparkQA · 2021-06-17T08:40:42Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44435/

zhengruifeng · 2021-06-18T05:50:30Z

Thanks @huaxingao @srowen for reviewing!

huaxingao · 2021-06-18T05:50:44Z

Merged to master, thanks！

HyukjinKwon · 2021-06-18T07:45:06Z

Hm, the tests didn't pass:

2021-06-16T16:16:45.6226175Z ======================================================================
2021-06-16T16:16:45.6227313Z ERROR [7.047s]: test_raw_and_probability_prediction (pyspark.ml.tests.test_algorithms.MultilayerPerceptronClassifierTest)
2021-06-16T16:16:45.6228919Z ----------------------------------------------------------------------
2021-06-16T16:16:45.6229472Z Traceback (most recent call last):
2021-06-16T16:16:45.6270603Z   File "/__w/spark/spark/python/pyspark/ml/tests/test_algorithms.py", line 89, in test_raw_and_probability_prediction
2021-06-16T16:16:45.6271739Z     self.assertTrue(np.allclose(result.rawPrediction, expected_rawPrediction, rtol=0.1))
2021-06-16T16:16:45.6272750Z AssertionError: False is not true
2021-06-16T16:16:45.6273240Z 
2021-06-16T16:16:45.6274058Z ----------------------------------------------------------------------

and seems like it fails. I am reverting this for now.

huaxingao · 2021-06-18T18:26:50Z

Sorry, I will be more careful next time.

zhengruifeng · 2021-06-21T02:32:59Z

@HyukjinKwon It is OK to revert this PR, I will send a new one which fix this failed case.
But It is somewhat surprising to me that it showed This patch passes all tests. with a failed pyspark test.

HyukjinKwon · 2021-06-21T03:22:11Z

Ah, Jenkins and GA have a a bit different env some times. GA is more correct in general fwiw

zhengruifeng · 2021-06-21T08:28:12Z

OK, we will to be more careful @HyukjinKwon

init

cbcb584

github-actions bot added the ML label Jun 16, 2021

HyukjinKwon changed the title ~~[SPARK-35678][ML][FOLLOWUP] use softmax in NB~~ [SPARK-35678][ML][FOLLOWUP] Use softmax in NB Jun 16, 2021

srowen reviewed Jun 16, 2021

View reviewed changes

support offset & step & output

6ee9054

zhengruifeng commented Jun 16, 2021

View reviewed changes

github-actions bot added the MLLIB label Jun 16, 2021

srowen approved these changes Jun 16, 2021

View reviewed changes

zhengruifeng changed the title ~~[SPARK-35678][ML][FOLLOWUP] Use softmax in NB~~ [SPARK-35678][ML][FOLLOWUP] softmax support offset and step Jun 17, 2021

srowen approved these changes Jun 17, 2021

View reviewed changes

huaxingao approved these changes Jun 18, 2021

View reviewed changes

huaxingao closed this in fdf86fd Jun 18, 2021

zhengruifeng deleted the softmax__followup branch June 18, 2021 05:50

zhengruifeng mentioned this pull request Jun 21, 2021

[SPARK-35678][ML][FOLLOWUP] softmax support offset and step #32991

Closed

[SPARK-35678][ML][FOLLOWUP] softmax support offset and step #32927

[SPARK-35678][ML][FOLLOWUP] softmax support offset and step #32927

Uh oh!

Conversation

zhengruifeng commented Jun 16, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Jun 16, 2021

Uh oh!

SparkQA commented Jun 16, 2021

Uh oh!

SparkQA commented Jun 16, 2021

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

zhengruifeng commented Jun 16, 2021

Uh oh!

zhengruifeng Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

zhengruifeng Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

zhengruifeng Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

zhengruifeng Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 16, 2021

Uh oh!

SparkQA commented Jun 16, 2021

Uh oh!

SparkQA commented Jun 16, 2021

Uh oh!

zhengruifeng commented Jun 17, 2021

Uh oh!

SparkQA commented Jun 17, 2021

Uh oh!

SparkQA commented Jun 17, 2021

Uh oh!

SparkQA commented Jun 17, 2021

Uh oh!

zhengruifeng commented Jun 18, 2021

Uh oh!

huaxingao commented Jun 18, 2021

Uh oh!

HyukjinKwon commented Jun 18, 2021

Uh oh!

huaxingao commented Jun 18, 2021

Uh oh!

zhengruifeng commented Jun 21, 2021

Uh oh!

HyukjinKwon commented Jun 21, 2021

Uh oh!

zhengruifeng commented Jun 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants