[SPARK-25758][ML] Deprecate computeCost on BisectingKMeans #22756

mgaido91 · 2018-10-17T15:06:58Z

What changes were proposed in this pull request?

The PR proposes to deprecate the computeCost method on BisectingKMeans in favor of the adoption of ClusteringEvaluator in order to evaluate the clustering.

How was this patch tested?

NA

SparkQA · 2018-10-17T16:27:17Z

Test build #97495 has finished for PR 22756 at commit 4761989.

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

mgaido91 · 2018-10-17T16:45:11Z

cc @holdenk @srowen

srowen

I see, this is a little more than just deprecating a few methods. Is this basically the same change as for KMeans?

srowen · 2018-10-17T17:25:13Z

python/pyspark/ml/clustering.py

        return [c.toArray() for c in self._call_java("clusterCenters")]

-    @since("2.0.0")
-    def computeCost(self, dataset):


Hm, can you actually remove this, vs just deprecate it?

sorry, this was not intended, I am fixing this.

mgaido91

yes, this is the same change which was done there, ie. deprecate the computeCost, but offer the cost on the training dataset in the summary, in order for the users to be able to still get it.

In addition, I also edited the examples so that they don't include deprecated methods.

mgaido91 · 2018-10-17T17:42:58Z

python/pyspark/ml/clustering.py

        return [c.toArray() for c in self._call_java("clusterCenters")]

-    @since("2.0.0")
-    def computeCost(self, dataset):


sorry, this was not intended, I am fixing this.

SparkQA · 2018-10-17T19:06:05Z

Test build #97501 has finished for PR 22756 at commit ed235f2.

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

dongjoon-hyun · 2018-10-17T23:43:36Z

mllib/src/main/scala/org/apache/spark/ml/clustering/BisectingKMeans.scala

+   *             instead. You can also get the cost on the training dataset in the summary.
   */
  @Since("2.0.0")
+  @deprecated("This method is deprecated and will be removed in 3.0.0. Use ClusteringEvaluator " +


I'm wondering if this PR is a blocker for Spark 2.4. According to JIRA desciption (Improvement/Minor), we cannot remove this at 3.0.0 because we cannot announce deprecations before 3.0.0. So, this PR looks invalid to me.

cc @cloud-fan since he is a release manager.

It looks reasonable to me to deprecate it in 2.4 so that we can remove it in 3.0, if this is the last one. Then we can have a consistent ML API in 3.0 after removing these deprecated APIs.

Thank you for the decision, @cloud-fan ! So, this is one of the task ML API auditing.

yes this is the last one.

Then we can have a consistent ML API in 3.0 after removing these deprecated APIs.

Yes, that's my goal in targeting this for 2.4.

Thanks.

dongjoon-hyun · 2018-10-18T06:28:02Z

examples/src/main/java/org/apache/spark/examples/ml/JavaBisectingKMeansExample.java

+    ClusteringEvaluator evaluator = new ClusteringEvaluator();
+
+    double silhouette = evaluator.evaluate(predictions);
+    System.out.println("Silhouette with squared euclidean distance = " + silhouette);


@mgaido91 .
If we are going to change all ml examples for deprecation, we had better change the following, too. And, could you check if we had another instances?

https://github.com/apache/spark/blob/master/examples/src/main/python/ml/bisecting_k_means_example.py#L45

# Evaluate clustering. cost = model.computeCost(dataset)

thanks I'll do @dongjoon-hyun

dongjoon-hyun · 2018-10-18T06:30:02Z

@mgaido91 . If you don't mind, could you split this PR into two PRs? One is adding deprecation annotation only. The other is adding new API and updating all examples?

mgaido91 · 2018-10-18T09:45:52Z

@dongjoon-hyun sure, thanks. I'll update asap. Thanks.

SparkQA · 2018-10-18T11:21:39Z

Test build #97525 has finished for PR 22756 at commit d5fddb5.

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

cloud-fan · 2018-10-18T12:10:27Z

LGTM

dongjoon-hyun

+1, LGTM.

WeichenXu123 · 2018-10-18T16:38:55Z

LGTM. thanks!

dongjoon-hyun · 2018-10-18T17:31:34Z

Merged to master/branch-2.4.

## What changes were proposed in this pull request? The PR proposes to deprecate the `computeCost` method on `BisectingKMeans` in favor of the adoption of `ClusteringEvaluator` in order to evaluate the clustering. ## How was this patch tested? NA Closes #22756 from mgaido91/SPARK-25758. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit c296254) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

dongjoon-hyun · 2018-10-18T17:34:12Z

Thank you, @mgaido91 and all!

holdenk · 2018-10-19T17:04:10Z

I'm seeing this linked from #22764 and I'm wondering if we need to revert this. If the information is not actually available where we tell folks it is I think we need to revert this especially since we are in the middle of the release process. Or raise SPARK-25765 to blocker release blocker.

Have I misunderstood the situation here?

dongjoon-hyun · 2018-10-19T17:22:39Z

I also understand today's situation and agree with @holdenk 's thought about SPARK-25765 as a blocker. Ping @cloud-fan since you are a release manager. How can we proceed SPARK-25765?

Maybe, this is due to Preparing Spark release v2.4.0-rc4 which happen two hours ago. We are in the middle of unstable situation.

Also, cc @gatorsmile .

gatorsmile · 2018-10-19T21:40:43Z

cc @mengxr WDYT? It does not sound a blocker to me.

mengxr · 2018-10-19T21:49:53Z

We have to revert this PR in branch-2.4. It is not a blocker and we shouldn't merge it to branch-2.4 this late in this already delayed release.

gatorsmile · 2018-10-19T21:50:39Z

Let me revert it. Thanks!

gatorsmile · 2018-10-19T22:15:57Z

Done

cloud-fan · 2018-10-20T05:06:25Z

shall we revert it from master as well? At least we need to update the message This method is deprecated and will be removed in 3.0.0.

mgaido91 · 2018-10-20T08:26:24Z

yes, I agree, if we are not going to deprecate it in 2.4, we need to revert also on master because of @cloud-fan's comment.

This would mean we won't have coherency with KMeans though, which is not that good IMHO.
Thanks.

cloud-fan · 2018-10-21T01:13:18Z

reverted from master. Let's move the discussion to #22764

## What changes were proposed in this pull request? The PR proposes to deprecate the `computeCost` method on `BisectingKMeans` in favor of the adoption of `ClusteringEvaluator` in order to evaluate the clustering. ## How was this patch tested? NA Closes apache#22756 from mgaido91/SPARK-25758. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

[SPARK-25758][ML] Deprecate computeCost on BisectingKMeans

4761989

srowen reviewed Oct 17, 2018

View reviewed changes

mgaido91 commented Oct 17, 2018

View reviewed changes

undo unrelated changes

ed235f2

dongjoon-hyun reviewed Oct 17, 2018

View reviewed changes

dongjoon-hyun reviewed Oct 18, 2018

View reviewed changes

revert non-deprecation changes

d5fddb5

mgaido91 force-pushed the SPARK-25758 branch from 8526b2b to d5fddb5 Compare October 18, 2018 10:11

dongjoon-hyun approved these changes Oct 18, 2018

View reviewed changes

cloud-fan mentioned this pull request Oct 18, 2018

[SPARK-25765][ML] Add training cost to BisectingKMeans summary #22764

Closed

asfgit closed this in c296254 Oct 18, 2018

cloud-fan mentioned this pull request Oct 20, 2018

[SPARK-25764][ML][EXAMPLES] Update BisectingKMeans example to use ClusteringEvaluator #22763

Closed

mgaido91 mentioned this pull request Oct 28, 2018

[SPARK-25758][ML] Deprecate computeCost in BisectingKMeans #22869

Closed

mgaido91 mentioned this pull request Nov 20, 2018

[SPARK-25867][ML] Remove KMeans computeCost #22875

Closed

[SPARK-25758][ML] Deprecate computeCost on BisectingKMeans #22756

[SPARK-25758][ML] Deprecate computeCost on BisectingKMeans #22756

Uh oh!

Conversation

mgaido91 commented Oct 17, 2018 • 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 Oct 17, 2018

Uh oh!

mgaido91 commented Oct 17, 2018

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

srowen Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

mgaido91 Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

mgaido91 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgaido91 Oct 17, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 17, 2018

Uh oh!

dongjoon-hyun Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgaido91 Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgaido91 Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 18, 2018

Uh oh!

mgaido91 commented Oct 18, 2018

Uh oh!

SparkQA commented Oct 18, 2018

Uh oh!

cloud-fan commented Oct 18, 2018

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

WeichenXu123 commented Oct 18, 2018

Uh oh!

dongjoon-hyun commented Oct 18, 2018

Uh oh!

dongjoon-hyun commented Oct 18, 2018

Uh oh!

holdenk commented Oct 19, 2018

Uh oh!

dongjoon-hyun commented Oct 19, 2018

Uh oh!

gatorsmile commented Oct 19, 2018

Uh oh!

mengxr commented Oct 19, 2018

Uh oh!

gatorsmile commented Oct 19, 2018

Uh oh!

gatorsmile commented Oct 19, 2018

mgaido91 commented Oct 17, 2018 •

edited

Loading

mgaido91 left a comment •

edited

Loading

dongjoon-hyun Oct 17, 2018 •

edited

Loading

dongjoon-hyun Oct 18, 2018 •

edited

Loading

dongjoon-hyun Oct 18, 2018 •

edited

Loading