[SPARK-19110][ML][MLLIB]:DistributedLDAModel returns different logPrior for original and loaded model #16491
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Jenkins, retest this please. |
|
Test build #70992 has finished for PR 16491 at commit
|
felixcheung
reviewed
Jan 6, 2017
| val trainingLogLikelihood2 = | ||
| model2.asInstanceOf[DistributedLDAModel].trainingLogLikelihood | ||
| assert(logPrior ~== logPrior2 absTol 1e-6) | ||
| assert(trainingLogLikelihood ~== trainingLogLikelihood2 absTol 1e-6) |
Member
There was a problem hiding this comment.
should we check trainingLogLikelihood and logPrior are not changing for LocalLDAModel?
Contributor
Author
There was a problem hiding this comment.
logLikelihood and logPrior are only for distributed model.
Member
There was a problem hiding this comment.
right - I mean that they are not persisted & loaded into an unexpected but valid value (!= Double.NaN)
Contributor
Author
There was a problem hiding this comment.
LocalLDAModel doesn't extend DistributedLDAModel and vice versa. I am not clear how to check trainingLogLikelihood and logPrior in LocalLDAModel.
Member
There was a problem hiding this comment.
Ok, I guess I remember this wrong because of the other PR.
Member
|
@jkbradley @yanboliang please have a look |
Member
|
Yikes, thanks for fixing this! |
asfgit
pushed a commit
that referenced
this pull request
Jan 7, 2017
…or for original and loaded model
## What changes were proposed in this pull request?
While adding DistributedLDAModel training summary for SparkR, I found that the logPrior for original and loaded model is different.
For example, in the test("read/write DistributedLDAModel"), I add the test:
val logPrior = model.asInstanceOf[DistributedLDAModel].logPrior
val logPrior2 = model2.asInstanceOf[DistributedLDAModel].logPrior
assert(logPrior === logPrior2)
The test fails:
-4.394180878889078 did not equal -4.294290536919573
The reason is that `graph.vertices.aggregate(0.0)(seqOp, _ + _)` only returns the value of a single vertex instead of the aggregation of all vertices. Therefore, when the loaded model does the aggregation in a different order, it returns different `logPrior`.
Please refer to #16464 for details.
## How was this patch tested?
Add a new unit test for testing logPrior.
Author: [email protected] <[email protected]>
Closes #16491 from wangmiao1981/ldabug.
(cherry picked from commit 036b503)
Signed-off-by: Joseph K. Bradley <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Jan 7, 2017
…or for original and loaded model
## What changes were proposed in this pull request?
While adding DistributedLDAModel training summary for SparkR, I found that the logPrior for original and loaded model is different.
For example, in the test("read/write DistributedLDAModel"), I add the test:
val logPrior = model.asInstanceOf[DistributedLDAModel].logPrior
val logPrior2 = model2.asInstanceOf[DistributedLDAModel].logPrior
assert(logPrior === logPrior2)
The test fails:
-4.394180878889078 did not equal -4.294290536919573
The reason is that `graph.vertices.aggregate(0.0)(seqOp, _ + _)` only returns the value of a single vertex instead of the aggregation of all vertices. Therefore, when the loaded model does the aggregation in a different order, it returns different `logPrior`.
Please refer to #16464 for details.
## How was this patch tested?
Add a new unit test for testing logPrior.
Author: [email protected] <[email protected]>
Closes #16491 from wangmiao1981/ldabug.
(cherry picked from commit 036b503)
Signed-off-by: Joseph K. Bradley <[email protected]>
cmonkey
pushed a commit
to cmonkey/spark
that referenced
this pull request
Jan 9, 2017
…or for original and loaded model
## What changes were proposed in this pull request?
While adding DistributedLDAModel training summary for SparkR, I found that the logPrior for original and loaded model is different.
For example, in the test("read/write DistributedLDAModel"), I add the test:
val logPrior = model.asInstanceOf[DistributedLDAModel].logPrior
val logPrior2 = model2.asInstanceOf[DistributedLDAModel].logPrior
assert(logPrior === logPrior2)
The test fails:
-4.394180878889078 did not equal -4.294290536919573
The reason is that `graph.vertices.aggregate(0.0)(seqOp, _ + _)` only returns the value of a single vertex instead of the aggregation of all vertices. Therefore, when the loaded model does the aggregation in a different order, it returns different `logPrior`.
Please refer to apache#16464 for details.
## How was this patch tested?
Add a new unit test for testing logPrior.
Author: [email protected] <[email protected]>
Closes apache#16491 from wangmiao1981/ldabug.
ghost
pushed a commit
to dbtsai/spark
that referenced
this pull request
Jan 13, 2017
…nd logLikelihood of DistributedLDAModel in MLLIB ## What changes were proposed in this pull request? apache#16491 added the fix to mllib and a unit test to ml. This followup PR, add unit tests to mllib suite. ## How was this patch tested? Unit tests. Author: [email protected] <[email protected]> Closes apache#16524 from wangmiao1981/ldabug.
uzadude
pushed a commit
to uzadude/spark
that referenced
this pull request
Jan 27, 2017
…or for original and loaded model
## What changes were proposed in this pull request?
While adding DistributedLDAModel training summary for SparkR, I found that the logPrior for original and loaded model is different.
For example, in the test("read/write DistributedLDAModel"), I add the test:
val logPrior = model.asInstanceOf[DistributedLDAModel].logPrior
val logPrior2 = model2.asInstanceOf[DistributedLDAModel].logPrior
assert(logPrior === logPrior2)
The test fails:
-4.394180878889078 did not equal -4.294290536919573
The reason is that `graph.vertices.aggregate(0.0)(seqOp, _ + _)` only returns the value of a single vertex instead of the aggregation of all vertices. Therefore, when the loaded model does the aggregation in a different order, it returns different `logPrior`.
Please refer to apache#16464 for details.
## How was this patch tested?
Add a new unit test for testing logPrior.
Author: [email protected] <[email protected]>
Closes apache#16491 from wangmiao1981/ldabug.
uzadude
pushed a commit
to uzadude/spark
that referenced
this pull request
Jan 27, 2017
…nd logLikelihood of DistributedLDAModel in MLLIB ## What changes were proposed in this pull request? apache#16491 added the fix to mllib and a unit test to ml. This followup PR, add unit tests to mllib suite. ## How was this patch tested? Unit tests. Author: [email protected] <[email protected]> Closes apache#16524 from wangmiao1981/ldabug.
cmonkey
pushed a commit
to cmonkey/spark
that referenced
this pull request
Feb 15, 2017
…nd logLikelihood of DistributedLDAModel in MLLIB ## What changes were proposed in this pull request? apache#16491 added the fix to mllib and a unit test to ml. This followup PR, add unit tests to mllib suite. ## How was this patch tested? Unit tests. Author: [email protected] <[email protected]> Closes apache#16524 from wangmiao1981/ldabug.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
While adding DistributedLDAModel training summary for SparkR, I found that the logPrior for original and loaded model is different.
For example, in the test("read/write DistributedLDAModel"), I add the test:
val logPrior = model.asInstanceOf[DistributedLDAModel].logPrior
val logPrior2 = model2.asInstanceOf[DistributedLDAModel].logPrior
assert(logPrior === logPrior2)
The test fails:
-4.394180878889078 did not equal -4.294290536919573
The reason is that
graph.vertices.aggregate(0.0)(seqOp, _ + _)only returns the value of a single vertex instead of the aggregation of all vertices. Therefore, when the loaded model does the aggregation in a different order, it returns differentlogPrior.Please refer to #16464 for details.
How was this patch tested?
Add a new unit test for testing logPrior.