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

Speed up sample_topp #274

Closed
wants to merge 3 commits into from
Closed

Conversation

dror-cyata
Copy link

This PR should allow using topp logic while avoiding the significant performance hit

Sampling probabilities tend to be sharp, so filtering out low probabilities before sorting the list result with significant time improvement for sample_topp.
We use a threshold that guaranties the filtered entries do not belong to topp (except for maybe a very rare edge case, need to think on it), but anyway we verify it to allow higher threshold.

run.c Outdated Show resolved Hide resolved
@kroggen
Copy link
Contributor

kroggen commented Aug 12, 2023

I suspect that #270 is faster

@dror-cyata
Copy link
Author

I suspect that #270 is faster

Just looked on #270. Both indeed look similar and filtering out entries with low probabilities, so they should take similar time (threshold of 1e-5 is used there, and ~3e-6 is used here, by default). However, the threshold used there is arbitrary, and here we used a threshold that assures that the filtered out entries do not belong to topp. Note that in #270 there might be edge cases where entries that are part of topp are unintentionally filtered out.

@rdentato
Copy link
Contributor

I agree with @dror-cyata, my constant in PR#270 was arbitrary and I have now replaced it with (1-topp)/(n-1).
The benefit are still substantial. I hope @karpathy will merge one of these PR (there is also #276 on the same topic)

@karpathy
Copy link
Owner

Merged #276 ty

@karpathy karpathy closed this Aug 14, 2023
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