[SPARK-21741][ML][PySpark] Python API for DataFrame-based multivariate summarizer #20695
Conversation
|
Test build #87778 has finished for PR 20695 at commit
|
|
Test build #87782 has finished for PR 20695 at commit
|
|
Test build #87784 has finished for PR 20695 at commit
|
|
2.4.0? |
|
Test build #87816 has finished for PR 20695 at commit
|
MrBago
left a comment
MrBago
left a comment
There was a problem hiding this comment.
One small comment, otherwise LGTM.
python/pyspark/ml/stat.py
Outdated
| return Summarizer(js) | ||
|
|
||
| @since("2.4.0") | ||
| def summary(self, featureCol, weightCol=None): |
There was a problem hiding this comment.
We might want to move the "summary" method into another class, and have Summary only contain static methods. That will help with autocomplete so that it's clear that you're not meant to do Summery.metrics("min").mean(features).
There was a problem hiding this comment.
Sounds reasonable.
|
Test build #88463 has finished for PR 20695 at commit
|
|
Test build #89167 has finished for PR 20695 at commit
|
jkbradley
left a comment
jkbradley
left a comment
There was a problem hiding this comment.
Looks good, just a few comments. Thanks!
python/pyspark/ml/stat.py
Outdated
| return SummarizerBuilder(js) | ||
|
|
||
|
|
||
| class SummarizerBuilder(object): |
There was a problem hiding this comment.
This name needs to match its Scala equivalent: "SummaryBuilder"
There was a problem hiding this comment.
Also, shouldn't we use JavaWrapper for this? That will clean up when this object is destroyed.
python/pyspark/ml/stat.py
Outdated
| self._js = js | ||
|
|
||
| @since("2.4.0") | ||
| def summary(self, featureCol, weightCol=None): |
There was a problem hiding this comment.
ditto: naming should match Scala: "featuresCol"
| def summary(self, featureCol, weightCol=None): | ||
| """ | ||
| Returns an aggregate object that contains the summary of the column with the requested | ||
| metrics. |
There was a problem hiding this comment.
Let's copy the docs for arguments & return value from Scala
|
Test build #89331 has finished for PR 20695 at commit
|
|
Test build #89333 has finished for PR 20695 at commit
|
jkbradley
left a comment
jkbradley
left a comment
There was a problem hiding this comment.
Thanks for updates, just 1 new comment
python/pyspark/ml/stat.py
Outdated
| """ | ||
| def __init__(self, js): | ||
| self._js = js |
There was a problem hiding this comment.
This should call the super's init method, and it should store js in _java_obj (which is set in the JavaWrapper init).
|
Test build #89422 has finished for PR 20695 at commit
|
|
LGTM |
5 participants
What changes were proposed in this pull request?
Python API for DataFrame-based multivariate summarizer.
How was this patch tested?
doctest added.