[SPARK-14894][PySpark] Add result summary api to Gaussian Mixture #12675
Conversation
|
@wangmiao1981 @jkbradley Please help review this PR |
python/pyspark/ml/clustering.py
Outdated
There was a problem hiding this comment.
This PyDoc seems oddly formatted in terms of line breaks.
GayathriMurali
force-pushed
the
SPARK-14894
branch
from
f3114ed to
11f0574
Compare
|
@jkbradley Can you please ok to test this |
|
@holdenk I fixed the pydoc style issue. Can you please help review this? |
python/pyspark/ml/clustering.py
Outdated
There was a problem hiding this comment.
this also seems strangely formatted - did you mean trainingSummary is None. ?
|
So did a quick look and it seems like it might not be lining up with the Scala implementation (or I just haven't had enough coffee this morning). It might be worthwhile to doublecheck against the scaladoc and also run your tests locally if you haven't done that. |
GayathriMurali
force-pushed
the
SPARK-14894
branch
from
11f0574 to
2e226ce
Compare
|
@holdenk I checked the ScalaDoc and removed the evaluate method. Thanks for pointing it out. Can you please help review |
|
@jkbradley @holdenk Can you please help review? |
|
@jkbradley This PR has been open >30days. Can you please help review? |
|
ok to test |
|
Test build #60707 has finished for PR 12675 at commit
|
GayathriMurali
force-pushed
the
SPARK-14894
branch
from
2e226ce to
7d16a23
Compare
|
Test build #60739 has finished for PR 12675 at commit
|
|
Test build #60748 has finished for PR 12675 at commit
|
|
Test build #60820 has finished for PR 12675 at commit
|
|
Test build #60821 has finished for PR 12675 at commit
|
|
|
||
| def test_gaussian_mixture_summary(self): | ||
| from pyspark.mllib.linalg import Vectors | ||
| df = self.spark.read.format("libsvm").load("data/mllib/sample_kmeans_data.txt") |
There was a problem hiding this comment.
Please let me know if its ok to load data from a file when all other test cases uses hard coded data values. I tried with a sparse vector and fit gave me an error for the data format.
There was a problem hiding this comment.
I think its ok to load a dataset which is already used for testing else where.
|
@MLnick It would be great if you can help review this. |
holdenk
left a comment
holdenk
left a comment
There was a problem hiding this comment.
Are you still interested in this PR @GayathriMurali ? If you've got the time to look at some of my notes around the PyDocs that would be cool - but we can also try pinging @davies & @MLnick to see if they are available to review once those comments are looked at :)
| return self._call_java("gaussiansDF") | ||
|
|
||
| @property | ||
| @since("2.0.0") |
There was a problem hiding this comment.
Since we've shipped 2.0 we will need to update this to 2.1 (same with the versionAdded notes)
|
Hi @GayathriMurali, it seems inactive for the review comments for few months. Should this be better closed for now if you are not able to proceed this further? |
|
@GayathriMurali If you are not able to proceed, I can take over. Thanks! |
|
#15777 has resolved this issue. We should close this one. |
|
@HyukjinKwon @srowen This should be closed. Thanks! |
|
@wangmiao1981 Wow, thank you for your efforts to verify it! |
6 participants
What changes were proposed in this pull request?
Add summary API to Gaussian Mixture
How was this patch tested?
Added unit test case to test summary information