Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pow is bottleneck, use faster pow! #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SimonDanisch
Copy link

Since the algorithm doesn't seem to need much precision, it works out pretty well:
image
It's 2.5x faster with fast_pow
if the loss in precision isn't acceptable ( i think it's a pretty rough approximation), ironically removing @fastmath also speed things up, since then it uses llvm.pow, which seems to optimize better!

Since this algorithm doesn't seem to need to be precise, it works out pretty well:
@codecov-io
Copy link

codecov-io commented Jan 9, 2019

Codecov Report

Merging #2 into master will increase coverage by 2.63%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #2      +/-   ##
=========================================
+ Coverage   83.76%   86.4%   +2.63%     
=========================================
  Files           3       3              
  Lines         117     125       +8     
=========================================
+ Hits           98     108      +10     
+ Misses         19      17       -2
Impacted Files Coverage Δ
src/umap_.jl 85.59% <84.21%> (+2.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb740f6...7f2bfa7. Read the comment docs.

@SimonDanisch
Copy link
Author

Interestingly, the speed up is much less pronounced when using the full dataset (benchmarked on only a small portion)... Guess then the nearest neighbor becomes more expensive.

@dillondaudert
Copy link
Owner

Thanks for making this PR!

I plan on doing some profiling to get a sense of how costly each component (including pow) is overall. It'd be nice to get a comparison of the three options you mention (current implementation, without @fastmath, and with fast_pow. I also have some more general thoughts.

I took a look at the source of fast_pow and one thing that doesn't sit well with me is that it doesn't seem rigorously tested, in the sense that the performance and error characteristics aren't really known (and therefore impossible to reason about). It's true SGD has tolerance for noise but it seems risky, I'll have to think about the potential impact more.

Another thing I'm wondering about is if the implementation of this function should reside in UMAP.jl itself. I haven't looked, but there might be a numerical methods-related package that is more appropriate (which we could then import from).

@dillondaudert
Copy link
Owner

@fastmath has been removed (f61ad40), which shaved about 20 seconds off the MNIST example runtime when I tested it.

@dillondaudert
Copy link
Owner

dillondaudert commented Jan 17, 2019

Had a look at the R implementation of UMAP today, looks like they use the same approximate power here - enabled by a keyword arg. It seems acceptable to me to incorporate it in a similar way here.

@SimonDanisch SimonDanisch mentioned this pull request May 4, 2020
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.

None yet

3 participants