-
Notifications
You must be signed in to change notification settings - Fork 854
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
Simplify byte_pair_merge #255
Conversation
parts.push((i, rank)); | ||
} | ||
parts.push((piece.len() - 1, Rank::MAX)); | ||
parts.push((piece.len(), Rank::MAX)); | ||
|
||
let get_rank = { | ||
#[inline(always)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you see any effect of the inlining here?
I didn't, and even the linter complained, this being a closure inheriting some paramters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I dimly remember it being useful in #31 (but it was also used in an additional place then). I can double check :-) Which linter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We drop optimisations enabled by ignoring single tokens.
the parts.len() > 3
means that once we're down to 2 tokens, we don't need more iterations, we don't have to try to merge it into a single token since we've already filtered those out - but that won't show up in the benchmarks, since that's basically constant time, so I agree, the code is simpler this way :)
Thanks for adding the comments back, please see my inline comments.
If you can, please add me as a coauthor here.
Thanks!
Co-authored-by: Lőrinc Pap <[email protected]>
Thank you for the original change and follow-up review! I've marked you as co-author on the commit :-) |
backport of openai#255 Co-authored-by: Shantanu <[email protected]> Co-authored-by: Lőrinc Pap <[email protected]>
Based on suggestion in #239 (specifically 8f5dd7d)
Like that commit, this:
Unlike that commit:
Let me know what you think! Once we figure this one out, we'll look at the linearithmic fix (I have some thoughts there, still doing some benchmarking).
Co-authored-by: @paplorinc