-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-8711] [ML] Add additional methods to PySpark ML tree models #7095
Conversation
Merged build triggered. |
Merged build started. |
ce343b9
to
849aabe
Compare
@mengxr Should we add wrapper around the |
Merged build triggered. |
Merged build started. |
Test build #36023 has started for PR 7095 at commit |
Merged build finished. Test FAILed. |
Test build #36023 has finished for PR 7095 at commit
|
Merged build finished. Test PASSed. |
@@ -409,6 +444,8 @@ class GBTRegressor(JavaEstimator, HasFeaturesCol, HasLabelCol, HasPredictionCol, | |||
... (0.0, Vectors.sparse(1, [], []))], ["label", "features"]) | |||
>>> gbt = GBTRegressor(maxIter=5, maxDepth=2) | |||
>>> model = gbt.fit(df) | |||
>>> model.treeWeights | |||
[1.0, 0.1, 0.1, 0.1, 0.1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Floating point equality is causing this test to fail. Try numpy.allclose
@feynmanliang Thanks. I have addressed your comments. |
Merged build triggered. |
Merged build started. |
Test build #36090 has started for PR 7095 at commit |
Test build #36090 has finished for PR 7095 at commit
|
Merged build finished. Test PASSed. |
Uh oh: looks like this is actually failing some tests:
This is the consequence of not having good test coverage on the test script... I'll fix this shortly. |
I've opened #7112 to hotfix the build issue which masked the test failure here. |
Jenkins, retest this please. |
Merged build triggered. |
Merged build started. |
Test build #36096 has started for PR 7095 at commit |
Test build #36096 has finished for PR 7095 at commit
|
Merged build finished. Test FAILed. |
Now that the test reporting is fixed, we can see the failure:
|
By the way, check out |
2fde31a
to
a6a27ca
Compare
Merged build triggered. |
I renamed pyTreeWeights to javaTreeWeights. |
Merged build triggered. |
Merged build started. |
Test build #36598 has started for PR 7095 at commit |
Test build #36598 has finished for PR 7095 at commit
|
Merged build finished. Test PASSed. |
@@ -70,6 +71,10 @@ private[ml] trait TreeEnsembleModel { | |||
/** Weights for each tree, zippable with [[trees]] */ | |||
def treeWeights: Array[Double] | |||
|
|||
/** Weights used by the python wrappers. */ | |||
// Note: An array cannot be returned directly due to serialization problems. | |||
def javaTreeWeights: Vector = Vectors.dense(treeWeights) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it package private. Java users do not really need it.
LGTM except that the utility method should be package private. |
done. I could not make it |
Merged build triggered. |
Merged build started. |
Merged build finished. Test FAILed. |
jenkins retest this please |
Merged build triggered. |
Merged build started. |
Test build #36652 has started for PR 7095 at commit |
Test build #36652 has finished for PR 7095 at commit
|
Merged build finished. Test FAILed. |
jenkins retest this please |
test this please |
Merged build triggered. |
Merged build started. |
Test build #36690 has started for PR 7095 at commit |
Test build #36690 has finished for PR 7095 at commit
|
Merged build finished. Test PASSed. |
Merged into master. Thanks! |
Add numNodes and depth to treeModels, add treeWeights to ensemble Models.
Add repr to all models.