[SPARK-14712][ML]spark.ml.LogisticRegressionModel.toString should sum… #12491
Conversation
|
|
||
| def __repr__(self): | ||
| return self._call_java("toString") | ||
|
|
There was a problem hiding this comment.
I think Python style asks for two new lines here (try running ./dev/lint-python locally :))
|
Thanks for taking the initiative on this PR - at first glance it seems like this approach might not quite work but its easier to tell with some tests - could you add a test case and run it locally? You add your test in LogisticRegressionSuite.scala for the scala test. As well you may find the linter tools ./dev/lint-scala & ./dev/lint-python to be useful :) |
| import org.apache.hadoop.fs.Path | ||
|
|
||
| import org.apache.spark.SparkException | ||
| import org.apache.spark.{SparkException} |
There was a problem hiding this comment.
This change seems unneeded.
python/pyspark/ml/classification.py
Outdated
| threshold=0.5, thresholds=None, probabilityCol="probability", | ||
| rawPredictionCol="rawPrediction", standardization=True, weightCol=None): | ||
| rawPredictionCol="rawPrediction", standardization=True, weightCol=None, | ||
| numFeatures=0, numClasses=0): |
There was a problem hiding this comment.
We should not expose numFeatures and numClasses as params which can be set by users. It should be property of **Model and be get directly from Scala by self._call_java("numFeatures").
|
ok with test |
| @Since("1.6.0") | ||
| override def write: MLWriter = new LogisticRegressionModel.LogisticRegressionModelWriter(this) | ||
|
|
||
| @Since("2.0.0") |
There was a problem hiding this comment.
We will need to update this since :)
| return BinaryLogisticRegressionSummary(java_blr_summary) | ||
|
|
||
| @since("2.0.0") | ||
| def __repr__(self): |
There was a problem hiding this comment.
It might make sense to include this in the docstring since it might be useful for people to see how to use it :)
|
To get this tested we will need a committer to say so. Maybe @jkbradley who created the JIRA can take a look / add you to the whitelist. |
|
@hujy Sorry for the delay! ok to test |
|
Test build #3520 has finished for PR 12491 at commit
|
|
@hujy Would you fix the test failure? If it is inactive, I would rather like to suggest to close this. |
## What changes were proposed in this pull request? This PR proposes to close PRs ... - inactive to the review comments more than a month - WIP and inactive more than a month - with Jenkins build failure but inactive more than a month - suggested to be closed and no comment against that - obviously looking inappropriate (e.g., Branch 0.5) To make sure, I left a comment for each PR about a week ago and I could not have a response back from the author in these PRs below: Closes apache#11129 Closes apache#12085 Closes apache#12162 Closes apache#12419 Closes apache#12420 Closes apache#12491 Closes apache#13762 Closes apache#13837 Closes apache#13851 Closes apache#13881 Closes apache#13891 Closes apache#13959 Closes apache#14091 Closes apache#14481 Closes apache#14547 Closes apache#14557 Closes apache#14686 Closes apache#15594 Closes apache#15652 Closes apache#15850 Closes apache#15914 Closes apache#15918 Closes apache#16285 Closes apache#16389 Closes apache#16652 Closes apache#16743 Closes apache#16893 Closes apache#16975 Closes apache#17001 Closes apache#17088 Closes apache#17119 Closes apache#17272 Closes apache#17971 Added: Closes apache#17778 Closes apache#17303 Closes apache#17872 ## How was this patch tested? N/A Author: hyukjinkwon <[email protected]> Closes apache#18017 from HyukjinKwon/close-inactive-prs.
7 participants
What changes were proposed in this pull request?
SPARK-14712
spark.mllib LogisticRegressionModel overrides toString to print a little model info. We should do the same in spark.ml and override repr in pyspark.
How was this patch tested?
LogisticRegressionSuite.scala