Skip to content

Conversation

@a1singh
Copy link
Contributor

@a1singh a1singh commented Nov 4, 2015

In file LDAOptimizer.scala:

line 441: since "idx" was never used, replaced unrequired zipWithIndex.foreach with foreach.

  •  nonEmptyDocs.zipWithIndex.foreach { case ((_, termCounts: Vector), idx: Int) =>
    
  •  nonEmptyDocs.foreach { case (_, termCounts: Vector) =>
    

line 441: since idx was never used, replaced unrequired zipWithIndex.foreach with foreach
@hvanhovell
Copy link
Contributor

Could you add the number of the JIRA ticket this relates to? See other PRs for an example.

@srowen
Copy link
Member

srowen commented Nov 4, 2015

@a1singh looks trivial, but OK. This isn't quite how you propose changes; this isn't a descriptive title for example. Please read https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark
Please also see if you can find other instances of the same problem that can be optimized.

@a1singh a1singh changed the title Update LDAOptimizer.scala [LDA]: Removed redundant zipWithIndex Nov 4, 2015
@a1singh a1singh changed the title [LDA]: Removed redundant zipWithIndex [Optimization][LDA]: Removed redundant zipWithIndex Nov 4, 2015
@a1singh a1singh changed the title [Optimization][LDA]: Removed redundant zipWithIndex [SPARK-11506][LDA]: Removed redundant operation in Online LDA implementation Nov 4, 2015
@a1singh a1singh changed the title [SPARK-11506][LDA]: Removed redundant operation in Online LDA implementation [SPARK-11506][MLLIB]: Removed redundant operation in Online LDA implementation Nov 4, 2015
@a1singh
Copy link
Contributor Author

a1singh commented Nov 5, 2015

@hvanhovell started a JIRA request and added the number to the title of this PR. In future, other similar micro-changes in MLLIB can be linked to this JIRA.

@srowen thanks for the link. I checked other files under Clustering, and they seem to be fine, with respect to this optimization.

@SparkQA
Copy link

SparkQA commented Nov 5, 2015

Test build #1987 has finished for PR 9456 at commit ba0879b.

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

@srowen
Copy link
Member

srowen commented Nov 5, 2015

Merged to master

@asfgit asfgit closed this in a94671a Nov 5, 2015
@a1singh
Copy link
Contributor Author

a1singh commented Nov 5, 2015

Thanks @srowen @hvanhovell

I would be glad to help with other activities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants