[SPARK-14370][MLLIB]removed duplicate generation of ids in OnlineLDAOptimizer

[pravingadakh]

What changes were proposed in this pull request?

Removed duplicated generation of ids in OnlineLDAOptimizer.

How was this patch tested?

tested with existing unit tests.

[srowen]

The match here becomes redundant right? both cases return v.values

[pravingadakh]

Yes it looks redundant, but values is not a member of parent trait Vector, it is defined at individual implementation level (i.e. DenseVector and SparseVector).

[srowen]

A-ha, understood about .values. This looks pretty reasonable. My only question is, does it make sense conceptually that this method also returns a list of IDs? it doesn't hurt much in practice, and it seems like there's a reasonable argument for it logically. We have one case where the caller needs it after all. Maybe finish this by documenting the three things returned from this method.

[pravingadakh]

I could document the returned values, but frankly I have no idea what those values are. I can see the documentation of first returned value gammad in the comments but none for others.

[srowen]

Ha fair enough. From poking around the internet I'm pretty sure the first is an estimate of gamma, the topic distribution (as the comments say) and the second are sufficient statistics for updating lambda.

[pravingadakh]

@jkbradley Could you please flag this PR for test build?

[srowen]

Jenkins test this please

[SparkQA]

Test build #55113 has finished for PR 12176 at commit fdc8bb0.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[hhbyyh]

Sean is right about the meanings of the return values.

[srowen]

@pravingadakh I'll merge if you'll doc the return value of this method. That further justifies the change.

[pravingadakh]

@srowen Added @return for the method.

[srowen]

This doesn't really describe what they are, but simply one place they're called. It doesn't need to be elaborate; see the explanations of what the firs two are above.

[pravingadakh]

@srowen How about this return doc:

Returns a tuple of gammad - estimate of gamma, the topic distribution, sstatsd - statistics for updating lambda and ids - list of termCounts vector indices

[srowen]

Sounds fine to me

[pravingadakh]

@srowen Updated the return doc accordingly.

[srowen]

@pravingadakh if you'll augment that doc a little bit more I'll merge this.

[jkbradley]

LGTM too pending the doc update (thanks for that)

[srowen]

Jenkins retest this please

[SparkQA]

Test build #55921 has finished for PR 12176 at commit 8107dc2.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Merged to master