[SPARK-24155][ML] Instrumentation improvements for clustering #21218
Conversation
lu-wang-dl
changed the title
|
Test build #90075 has finished for PR 21218 at commit
|
| @@ -423,6 +423,8 @@ class GaussianMixture @Since("2.0.0") ( | |||
| val summary = new GaussianMixtureSummary(model.transform(dataset), | |||
| $(predictionCol), $(probabilityCol), $(featuresCol), $(k), logLikelihood) | |||
| model.setSummary(Some(summary)) | |||
| instr.logNamedValue("logLikelihood", logLikelihood) | |||
| instr.logNamedValue("clusterSizes", summary.clusterSizes.toString) | |||
There was a problem hiding this comment.
The clusterSizes.toString will get an unreadable object address string.
We need to print the content of the array. I suggest add a method like:
def logNamedArray[T](array: Array[T]): Unit
There was a problem hiding this comment.
@WeichenXu123 The function clusterSizes.mkString(", ") could change the array to a string, separating each String in the array with comma. What do you think?
| @@ -378,6 +378,7 @@ class KMeans @Since("1.5.0") ( | |||
| model.transform(dataset), $(predictionCol), $(featuresCol), $(k)) | |||
|
|
|||
| model.setSummary(Some(summary)) | |||
| instr.logNamedValue("clusterSizes", summary.clusterSizes.toString) | |||
|
Test build #90152 has finished for PR 21218 at commit
|
| @@ -278,6 +279,7 @@ class BisectingKMeans @Since("2.0.0") ( | |||
| val summary = new BisectingKMeansSummary( | |||
| model.transform(dataset), $(predictionCol), $(featuresCol), $(k)) | |||
| model.setSummary(Some(summary)) | |||
| instr.logNamedValue("clusterSizes", summary.clusterSizes.mkString(", ")) | |||
There was a problem hiding this comment.
This requires parsing the result string if we want to do some analysis. How do we do this in other places? Do we always log in JSON format, e.g.? cc: @WeichenXu123 @jkbradley
There was a problem hiding this comment.
We log Params in JSON format, but we don't have a precedent for logging Array values. I'd be OK with ducking the issue by not logging this value, or with logging it as JSON. If the latter, then we could add a method Instrumentation.logNamedValue taking Array types.
There was a problem hiding this comment.
sg. @ludatabricks could you update the value to mkString("[", ",", "]") for now and leave a TODO to extend logNamedValue?
|
Test build #90529 has finished for PR 21218 at commit
|
|
LGTM. Merged into master. Thanks! |
What changes were proposed in this pull request?
changed the instrument for all of the clustering methods
How was this patch tested?
N/A
Please review http://spark.apache.org/contributing.html before opening a pull request.