Fix #1256 refactor Token Constants, test vocab init/build behaviour#1279
Conversation
profvjreddi
left a comment
There was a problem hiding this comment.
Great contribution, Peter — thanks for tackling #1256! 🎉
The core refactor (extracting '<UNK>' and '</w>' into Tokenizer.TOK_UNKNOWN / Tokenizer.TOK_EOW class constants) is solid, and the improved test_unit_char_tokenizer that verifies build_vocab actually overwrites the init vocabulary is a real improvement.
A few minor things I'll clean up post-merge:
-
Incomplete docstring migration — Some
>>> expected outputlines in docstrings still show raw'o</w>'while inputs were updated to useTokenizer.TOK_EOW. I'll make these consistent. -
ASCII diagrams → revert to literal values — The box-drawing diagrams are pedagogical visualizations meant to be read by students, not executed. Replacing
'o</w>'with'o'+Tokenizer.TOK_EOWin those diagrams adds cognitive load without maintainability benefit. I'll revert the diagram text back to concrete string values while keeping the constants in all executable code. -
f-string style nit — Minor inconsistency in assertion error message interpolation; I'll tidy that up.
Merging now — thanks again for the clean refactor!
- Revert ASCII diagrams to literal '</w>' and '<UNK>' values for pedagogical clarity (diagrams should be concrete, not symbolic) - Fix incomplete docstring migration where expected outputs still used raw string literals while inputs used Tokenizer.TOK_EOW - Standardize assertion error message interpolation style
|
@all-contributors please add @asgalon as a contributor for 🧑💻 Code in TinyTorch |
|
I've added @asgalon as a contributor to tinytorch! 🎉 Recognized for: doc The contributor list has been updated in:
We love recognizing our contributors! ❤️ |
Changes made:
By submitting this PR, you agree to release your contribution under the project's CC-BY-NC-ND license.