Skip to content

Handle short-token edge case in BPE pair selection#1004

Merged
rasbt merged 2 commits into
rasbt:mainfrom
amit-chaubey:fix/ch02-bpe-empty-pair-edge-case
Apr 11, 2026
Merged

Handle short-token edge case in BPE pair selection#1004
rasbt merged 2 commits into
rasbt:mainfrom
amit-chaubey:fix/ch02-bpe-empty-pair-edge-case

Conversation

@amit-chaubey
Copy link
Copy Markdown
Contributor

@amit-chaubey amit-chaubey commented Apr 9, 2026

Fixes #1002

Summary

  • Return None in find_freq_pair when fewer than 2 token IDs exist (or no pairs are found).
  • Keep train() from failing on short inputs by stopping gracefully.
  • Add a brief notebook note + tiny sanity checks.

Why

Short inputs could produce an empty pair set, causing max()/min() to fail before train() can break on pair_id is None.

Validation

  • find_freq_pair([]) -> None
  • find_freq_pair([42]) -> None
  • train(""), train("H"), and train("He") run without error.

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@rasbt
Copy link
Copy Markdown
Owner

rasbt commented Apr 11, 2026

Thanks for catching (and fixing) this issue @CLu1207 and @amit-chaubey

I checked the other more complete BPE notebook and this one should be fine.

Anyways, thanks for also adding the edge case tests.

@rasbt rasbt merged commit 59709e2 into rasbt:main Apr 11, 2026
13 checks passed
@amit-chaubey
Copy link
Copy Markdown
Contributor Author

Thanks for the review and the merge, much appreciated! 🙌

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.

find_freq_pair crashes when token_ids length < 2

2 participants