Skip to content

Conversation

@feynmanliang
Copy link
Contributor

  • Changes manual iteration loops into UFuncs, vectorized, and broadcasted operations
  • Refactors into more Scala idiomatic syntax (e.g. while and for -> foreach and maps, System.arraycopy -> ++)
  • Fixes various style issues (4-indent method args, wrap one-line ifs in {})
  • Adds comments and improves scaladocs (grammar, punctuation)

Notes to Reviewers

  • Using UFuncs simplifies ActivationFunction code significantly at the cost of more complicated implementations for crossEntropy, derivative, etc... I'm not sure if the tradeoff here is worth it
  • There is a slight performance hit in SoftmaxFunction.eval since we add an additional iteration over each column of x (previously computing exp(x - maxVal) and accumulating a sum were done at the same time whereas now the exp(x - maxVal) computation is done in one loop and the sum is computed after), but I feel that this is acceptable given the significant reduction in complexity. Thoughts?

CC @avulanov @mengxr

@SparkQA
Copy link

SparkQA commented Sep 7, 2015

Test build #42107 has finished for PR 8648 at commit 84f8bea.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 8, 2015

Test build #42113 has finished for PR 8648 at commit 22ba174.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 8, 2015

Test build #42121 has finished for PR 8648 at commit abdba81.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@feynmanliang feynmanliang changed the title [SPARK-10478][ML] Performance, organization, and style improvements for multi-layer perceptron [SPARK-10478][ML][WIP] Performance, organization, and style improvements for multi-layer perceptron Sep 8, 2015
@feynmanliang feynmanliang changed the title [SPARK-10478][ML][WIP] Performance, organization, and style improvements for multi-layer perceptron [SPARK-10478][ML] Performance, organization, and style improvements for multi-layer perceptron Sep 8, 2015
@SparkQA
Copy link

SparkQA commented Sep 8, 2015

Test build #42137 has finished for PR 8648 at commit f6731ff.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class BlockFetchException(messages: String, throwable: Throwable)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flatten might be expensive for array of large arrays, is not it?

@avulanov
Copy link
Contributor

avulanov commented Sep 8, 2015

@feynmanliang Thank you for reviewing the code! I made one pass. It seems that UFunc simplifies it a lot. However I am not sure about .flatten and .flatMap on array of large arrays. We need to perform performance comparison. Could you run the benchmark from https://github.com/avulanov/ann-benchmark before and after refactoring to see the difference?

@feynmanliang
Copy link
Contributor Author

@avulanov The benchmarking code is written against a WIP implementation; I sent you a PR for bringing it up to date.

LBFGS is taking significantly long time on my machine:
image

I've removed the flatten/flatMap changes from this PR and will save them for when I have more time to properly perf test.

@SparkQA
Copy link

SparkQA commented Sep 11, 2015

Test build #42348 has finished for PR 8648 at commit f56e2d5.

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

@avulanov
Copy link
Contributor

@feynmanliang I suggest using native BLAS for testing. It worth checking the impact of using UFunc as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feynmanliang Could you run some micro-benchmark on this function? I think this is the only place that might cause performance issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mengxr Local benchmarks here. Performance improves across the board except for (n=100000, k=50)

@feynmanliang
Copy link
Contributor Author

@mengxr added benchmarks, can you make another pass when you have a chance

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.

5 participants