Repository navigation
fix(itn/en): don't add up number words that aren't one number - #94
Conversation
grammatical_words_to_number added every ones or tens word to the running total whatever came before it, so "ten thirty" read as 40 and "twenty one forty two" as 2043. Since 9035284 the digit-by-digit reading also applied after "and" was stripped, so "five and six" read as 56. Track what the previous word filled (unit, tens or a scale word) and return None for a run that isn't one number; the sentence loop then falls back to shorter spans. Digit-by-digit reading now requires the run to contain no "and".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Alex-Wengg
left a comment
Alex-Wengg
left a comment
There was a problem hiding this comment.
Thanks, this is a good fix. cargo test --release passes locally (1,173 tests). I also compared 43 tricky inputs between main and this branch: every normalize_sentence change is an improvement ("ten thirty" 40 → 10 30, "it costs nine ninety nine" it costs 108 → it costs 9 99, "page twenty three twenty four" page 2027 → page 23 24). The real numbers I tried still read the same: 1984, 1905, 2021, 1776, 1105, 710, $5.50, 0.5, 21st, 12:05, 555-1212.
A few things outside the diff:
- Whole-input
normalize()now falls through to the telephone tagger for more inputs. "ten thirty" →103-0(was40), "twenty twenty" →202-0, "ten ten" →101-0, "twenty fifteen dollars" →201-5. The old answers were wrong too, so this isn't a correctness regression, but103-0looks worse than40. The telephone tagger already did this on main for "thirty five sixty two" →356-2. Could you mention it in the PR description, or should we open a follow-up issue about the telephone tagger accepting 4-digit inputs? - Two doc comments are now stale. The
words_to_numberdoc (cardinal.rs:169-170) and the comment atcardinal.rs:541say "twenty one forty two" must stay readable as the year 2042 fordate::parse_old_year.words_to_number("twenty one forty two")now returnsNone, andparse_old_yearactually split it as "twenty" + "one forty two" (giving 2043, never 2042). - The
KNOWN_ISSUEintest_boundary_very_long_input(tests/extensive_tests.rsaround line 1187) says repeated "twenty one" gets combined into bigger numbers. That is probably no longer true after this change, so the comment and its loose assertion could likely be tightened.
| assert_eq!(normalize_sentence("twenty one five dollars"), "$26"); | ||
| // "twenty one five dollars" is not 26 dollars (the money tagger no longer | ||
| // sums the prefix; what wins instead is the date tagger's old-year read). | ||
| assert_ne!(normalize_sentence("twenty one five dollars"), "$26"); |
There was a problem hiding this comment.
assert_ne! here passes for almost any output. Could this pin today's actual output, "2015 dollars", with a KNOWN_ISSUE note (the date tagger reads it as a year)? That way the next change to this path shows up as a test diff.
| found_number = true; | ||
| last = Last::Tens; | ||
| } else if word == "hundred" { | ||
| last = Last::Scale; |
There was a problem hiding this comment.
Scale words are still added up in any order: "two hundred three hundred" → 20300 and "one thousand two thousand" → 3000 (same as main). It's the same kind of not-one-number run this PR fixes. Rejecting a scale that isn't smaller than the previous one would cover it. Fine as a follow-up if you'd rather keep this PR narrow.
| if words.len() >= 2 && words.iter().all(|w| single_digit_char(w).is_some()) { | ||
| // Digits joined by "and" are a list ("five and six"), not a code. | ||
| if words.len() >= 2 | ||
| && words.len() == unfiltered.len() |
There was a problem hiding this comment.
Nit: unfiltered also drops "a", so this check turns off the digit-by-digit reading for an "a" as well, not just "and". I couldn't think of a real input where that matters, but the comment could say both.
| // A ones word may follow a tens word or a scale; a teen or tens word | ||
| // only a scale (or nothing). | ||
| #[derive(PartialEq)] | ||
| enum Last { |
There was a problem hiding this comment.
Nit: this enum would read a little more cleanly at module level next to ONES and TENS, instead of inside the function body.
- telephone: four to six spoken digits are not a phone number. With runs like "ten thirty" no longer summed, whole-input normalize() fell through to the telephone tagger and formatted them as "103-0" / "202-0"; they now stay as written, and "one two three four" / "nine four one zero five" read as 1234 / 94105 instead of "123-4" / "941-05". - cardinal: move the Last enum to module level, fix the doc comments that still said "twenty one forty two" reads as the year 2042, and note that the digit-by-digit check also excludes runs with "a". - tests: pin "twenty one five dollars" → "2015 dollars" as a KNOWN_ISSUE, and assert 100 × "twenty one" → 100 × "21" now that adjacent numbers aren't combined.
Spoken number words that don't form one number are added up by the cardinal reader:
grammatical_words_to_numberadds every ones or tens word to the running total whatever came before it (the KNOWN_ISSUE intests/extensive_tests.rsaround line 972), and since 9035284 the digit-by-digit reading also applies once "and" has been stripped, so "five and six" became 56.The change keeps track of what the previous word filled (unit, tens or a scale word) and returns
Nonefor a run that isn't one number: a ones word straight after a units word, or a teen/tens word after anything other than a scale word or the start. The sentence loop then falls back to shorter spans. Digit-by-digit reading now requires the run to contain no "and", so codes still read as before ("one two three" → 123), while "five and six" is a list.Every real number reads as before: 105, 2005, 2100, 1984, 112, 3909, 1,200,000.
Tests
test_runs_that_are_not_one_number.None, "twenty one forty two" 2043 → "21 42", "twenty one and forty two" → "21 and 42".cargo test --release: all 14 test binaries pass (1,173 tests); the NeMo data files intests/data/enpass at 100% before and after.Found while using the library for dictation in a macOS app; replaying 1,179 real dictations, it no longer turns lists like "four, five and six and seven" into one number.
🤖 Generated with Claude Code
Maintainer changes (fdedaa1)
normalize()fell through to the telephone tagger and formatted them as phone numbers. It now leaves them as written;normalize_sentencegives10 30etc. as before. The tagger's comment already said a phone number needs at least 7 digits (or 3 for a short code); only fewer than 3 was rejected.normalize()inputparse_old_yearsplit it as 20 + "one forty two" = 2043 anyway). The digit-by-digit comment also mentions "a". TheLastenum moved to module level.2015 dollarsas a KNOWN_ISSUE.test_boundary_very_long_inputnow asserts 100 × "twenty one" → 100 × "21" (main gave168×12 and2064), and its KNOWN_ISSUE is removed.Checks:
cargo test(1,174),--features ffi(1,182) and--releaseall pass, as dofst_parity(7/7) and the Swift FFI smoke test (142/142). The NeMo parity report is identical to main, with no regressions vs baseline.cargo fmt --checkpasses, and clippy reports only the warnings main already has.Not changed here (follow-ups)
words_to_numberalone made sentence mode worse:203 100, "ten thousand crore" →10 thousand 10000000. A proper fix needs the sentence span selection to prefer "two hundred" + "three hundred".normalize()drops unknown words through the telephone tagger ("twenty one five dollars" →215, and on main "call me at five five five one two one two" →555-1212), because its digit parser skips words it doesn't know.normalize_sentenceisn't affected.