Uh oh!
There was an error while loading. Please reload this page.
[SPARK-11938][ML] Expose numFeatures in all ML PredictionModel for Py… - #9936
[SPARK-11938][ML] Expose numFeatures in all ML PredictionModel for Py…#9936Lewuathe wants to merge 2 commits into
Conversation
SparkQA
commented
Nov 24, 2015
Test build #46609 has finished for PR 9936 at commit
|
Lewuathe
commented
Nov 28, 2015
@mengxr Could you review it? Thanks. |
There was a problem hiding this comment.
"The number of features" -> "The number of features used to train the model."
I think this is more explicit since, for things like decision trees, this property is what number of features were used in training and not necessarily what number of features the model incorporates in its prediction.
Nit: add period to follow docstring convention.
SparkQA
commented
Dec 2, 2015
Test build #47029 has finished for PR 9936 at commit
|
There was a problem hiding this comment.
Could we test numFeatures at each model that we can save cost of creating DataFrame and training model?
There was a problem hiding this comment.
Each trainer output corresponding models. So we cannot share training code but creating DataFrames can be separated each RegressorTest and ClassificationTest. I'll update so.
There was a problem hiding this comment.
@Lewuathe Sorry for late response. What I means is that we can add test for numFeatures in the current test cases of each algorithm. For example, we can add
>>> model.numFeatures
2
after here for LogisticRegressionModel.
The python doctest can also used as examples that users can get how to use numFeatures from the test case context.
Lewuathe
commented
Dec 12, 2015
@yanboliang I made test code to avoid recreation of datasets. |
SparkQA
commented
Dec 12, 2015
Test build #47604 has finished for PR 9936 at commit
|
SparkQA
commented
Dec 12, 2015
Test build #47605 has finished for PR 9936 at commit
|
There was a problem hiding this comment.
is there any reason for putting the import in the code?
There was a problem hiding this comment.
Vectors and StringIndexer is not used in any other place. It is better not to expand the scope in my though.
There was a problem hiding this comment.
I think either is fine. I'm sure Vectors will be used elsewhere in this file in the future, but am not sure about StringIndexer.
SparkQA
commented
Jan 23, 2016
Test build #49926 has finished for PR 9936 at commit
|
SparkQA
commented
Jan 23, 2016
Test build #49928 has finished for PR 9936 at commit
|
holdenk
commented
Jan 28, 2016
It seems like there is a lot of duplicated just calls to Java & PyDoc could we maybe make a HasNumFeatures or similar (similar to how we do this for params which we use in many places)? |
jkbradley
commented
Mar 29, 2016
Ping: Is this PR still active? (It would be nice) |
yanboliang
commented
Mar 31, 2016
@Lewuathe Do you have time to update this PR? If not, I can help. |
Lewuathe
commented
Apr 1, 2016
Sorry for late response. I'll do update. @yanboliang Thanks for taking care! |
Aggregate numFeatures property in HasNumFeaturesModel in base.py.
SparkQA
commented
Apr 1, 2016
Test build #54704 has finished for PR 9936 at commit
|
SparkQA
commented
Apr 1, 2016
Test build #54705 has finished for PR 9936 at commit
|
SparkQA
commented
Apr 2, 2016
Test build #54749 has finished for PR 9936 at commit
|
jkbradley
commented
Apr 20, 2016
@Lewuathe Things got busy over here too; sorry for the delay! Btw, can you please add "[PYTHON]" to the PR title? |
| It should be mixin with JavaModel. | ||
| """ | ||
| @property | ||
| @since("1.7.0") |
There was a problem hiding this comment.
Remove since tag. If this is added to another class in a later version, the since tag will be incorrect.
SparkQA
commented
May 12, 2016
Test build #58466 has finished for PR 9936 at commit
|
…Spark
see: https://issues.apache.org/jira/browse/SPARK-11938
Note: Test cases for
ml.regressionandml.classificationwere not implemented. The test written in this patch target only fornumFeatures. We can do enhancement of test case forml.regressionandml.classificationin separate JIRA.