You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/11/14 05:12:23 UTC
[GitHub] [spark] shahar603 opened a new pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
shahar603 opened a new pull request #30374:
URL: https://github.com/apache/spark/pull/30374
### What changes were proposed in this pull request?
Added an optional `initialModel` in the GaussianMixture class.
### Why are the changes needed?
To allow for non random initialization in ML's Gaussian Mixture. The algorithm's results can be affected due to the initialization and the existing random initialization may be limiting in some cases. This feature exists in MLLIB.
### Does this PR introduce _any_ user-facing change?
* A new method: `setInitialModel` in the `GaussianMixture` class
### How was this patch tested?
Yes, tests were added to the `GaussianMixtureSuite.scala` file.
The tests test:
* The ability to add the model
* The fact the GaussianMixture object fit data using the initial parameters
* That the GaussianMixture class validates that `K` is the same to the initial model's `K`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shahar603 commented on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
shahar603 commented on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-757128705
> in the .ml side, models do not support initialization for now. I think we need more discussion and design a general method to support them.
Do you think this fits the scope of this PR? Should I close it because it shouldn't be merged until such changes are implemented which in my understanding is TBD.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shahar603 edited a comment on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
shahar603 edited a comment on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-727200260
The reason the tests fail right now is due `Param[GaussianMixtureModel]`. GaussianMixtureModel cannot be serializable using `jsonEncoder`.
mllib/src/main/scala/org/apache/spark/ml/clustering/GaussianMixture.scala
So when the code tries to save/load the model, it fails because it can't serialize the GaussianMixtureModel object. Is there a way to fix this without changing the implementation of`jsonEncoder`? I believe changing `jsonEncoder` is beyond the scope of this PR and might require a new issue. What do you suggest?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-727147154
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #30374:
URL: https://github.com/apache/spark/pull/30374#discussion_r524364246
##########
File path: mllib/src/main/scala/org/apache/spark/ml/clustering/GaussianMixture.scala
##########
@@ -424,11 +463,11 @@ class GaussianMixture @Since("2.0.0") (
}
private def trainImpl(
- instances: RDD[(Vector, Double)],
- weights: Array[Double],
- gaussians: Array[(DenseVector, DenseVector)],
- numFeatures: Int,
- instr: Instrumentation): (Double, Int) = {
+ instances: RDD[(Vector, Double)],
Review comment:
Please revert all the whitespace changes first
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-822876336
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shahar603 commented on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
shahar603 commented on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-727200260
The reason the tests fails is due `Param[GaussianMixtureModel]`. GaussianMixtureModel cannot be serializable using `jsonEncoder`.
mllib/src/main/scala/org/apache/spark/ml/clustering/GaussianMixture.scala
So when the code tries to save/load the model, it fails because it can't serialize the GaussianMixtureModel object. Is there a way to fix this without changing the implementation of`jsonEncoder`? I believe changing `jsonEncoder` is beyond the scope of this PR and might require a new issue. What do you suggest?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shahar603 edited a comment on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
shahar603 edited a comment on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-727200260
The reason the tests fail right now is due `Param[GaussianMixtureModel]`. GaussianMixtureModel cannot be specialized using `jsonEncoder`.
mllib/src/main/scala/org/apache/spark/ml/clustering/GaussianMixture.scala
So when the code tries to save/load the model, it fails because it can't serialize the GaussianMixtureModel object. Is there a way to fix this without changing the implementation of`jsonEncoder`? I believe changing `jsonEncoder` is beyond the scope of this PR and might require a new issue. What do you suggest?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shahar603 edited a comment on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
shahar603 edited a comment on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-727200260
The reason the tests fail right now is due `Param[GaussianMixtureModel]`. GaussianMixtureModel cannot be specialized using `jsonEncoder`.
So when the code tries to save/load the model, it fails because it can't serialize the GaussianMixtureModel object. Is there a way to fix this without changing the implementation of`jsonEncoder`? I believe changing `jsonEncoder` is beyond the scope of this PR and might require a new issue. What do you suggest?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-736196878
in the .ml side, models do not support initialization for now. I think we need more discussion and design a general method to support them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #30374:
URL: https://github.com/apache/spark/pull/30374
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-727147056
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] shahar603 commented on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
shahar603 commented on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-757128705
> in the .ml side, models do not support initialization for now. I think we need more discussion and design a general method to support them.
Do you think this fits the scope of this PR? Should I close it because it shouldn't be merged until such changes are implemented which in my understanding is TBD.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #30374: [WIP][SPARK-33444][ML] Added support for Initial model in Gaussian Mixture Model in ML
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30374:
URL: https://github.com/apache/spark/pull/30374#issuecomment-727147056
Can one of the admins verify this patch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org