Skip to content

feat(ta): add cardinal ITN tagger, verbalizer and test cases - #2

Open
retheckj-star wants to merge 1 commit into
mainfrom
ta-itn-dummy-pr
Open

feat(ta): add cardinal ITN tagger, verbalizer and test cases#2
retheckj-star wants to merge 1 commit into
mainfrom
ta-itn-dummy-pr

Conversation

@retheckj-star

Copy link
Copy Markdown
Owner

Language: Tamil
Task: ITN (Inverse Text Normalization)

Summary:

  • Added Tamil ITN cardinal tagger.
  • Added Tamil ITN cardinal verbalizer.
  • Added TSV mappings for digits, zero, teens and tens.
  • Validated against the provided 28 core test cases.

DCO sign-off has been added and TODO instruction comments have been removed.

Signed-off-by: retheckj <retheckj@nvidia.com>

@mayuris-00 mayuris-00 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core task is signed off - all 28 cases pass and are correct.
Resolved since last review: TODO comments removed, DCO sign-off added, commit message corrected.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved. Correctly left empty as a package marker, per Section 5. No action required.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved. Correctly left empty as a package marker, per Section 5. No action required.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved.

Core implementation is correct: .invert() applied to all three sources, the union is well-formed, and the tagging wrapper (integer: "…" + add_tokens) is right.
The stale TODO comments have been removed - good.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved.

-TODO 3 is correct: pynini.closure(NEMO_NOT_QUOTE, 1) correctly captures the digit value between the quotes, and the token-deletion logic is right.
-Stale comment removed -good. No action required.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved.
Goof job!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Comment thread run_cardinal_tests.py

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved. Matches the spec's checker script and the required root location. No action required.

Comment thread test_cases_cardinal.txt

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Status: Approved. All 28 cases match Section 8 exactly and use the correct input~expected format.

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.

2 participants