Fixes issue 228

[zoobereq]

What does this PR do ?

The fix addresses one of the issues reported in #228, in particular:

text: Hier zoome ich auf die Läsion. Wir befinden uns also auf der 2D-Mammographie.
norm_text:Hier zoome ich auf die Läsion. Wir befinden uns also auf der 2D-Mammographie.
expected output: Hier zoome ich auf die Läsion. Wir befinden uns also auf der Zwei-D-Mammographie. (not sure)

The updated system correctly transduces these common hyphenated nominal compounds and can be easily expanded to include others.

Before your PR is "Ready for review"

Pre checks:

• Have you signed your commits? Use git commit -s to sign.
• Do all unittests finish successfully before sending PR?
  1. pytest or (if your machine does not have GPU) pytest --cpu from the root folder (given you marked your test cases accordingly @pytest.mark.run_only_on('CPU')).
  2. Sparrowhawk tests bash tools/text_processing_deployment/export_grammars.sh --MODE=test ...
• If you are adding a new feature: Have you added test cases for both pytest and Sparrowhawk here.
• Have you added __init__.py for every folder and subfolder, including data folder which has .TSV files?
• Have you followed codeQL results and removed unused variables and imports (report is at the bottom of the PR in github review box) ?
• Have you added the correct license header Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. to all newly added Python files?
• If you copied nemo_text_processing/text_normalization/en/graph_utils.py your header's second line should be Copyright 2015 and onwards Google, Inc.. See an example here.
• Remove import guards (try import: ... except: ...) if not already done.
• If you added a new language or a new feature please update the NeMo documentation (lives in different repo).
• Have you added your language support to tools/text_processing_deployment/pynini_export.py.

PR Type:

• New Feature
• Bugfix
• Documentation
• Test

If you haven't finished some of the above items you can still open "Draft" PR.

[tbartley94]

I think we can make this change a bit more powerful if you don't see any issue with suggestions.

Also, move this to whitelist class, not electronic since your current use cases are more whitelist styled.

[tbartley94]

Move to DE graph and import

[zoobereq]

Good idea.

[tbartley94]

is this valid for sparrowhawk?

[zoobereq]

Yes. It's a valid field in SP's Electronic semiotic class implementation, but since it's better to move everything to Whitelist, then I suppose it doesn't matter?

[tbartley94]

Mmm, what are our options with whitelist sparrowhawk wise?

[zoobereq]

Seeing how the SP proto doesn't even list Whitelist as a semiotic class -- rightfully so IMO -- the options are endless? The current implementation of Whitelist doesn't do much besides case-folding and deterministic routing. Moving the logic there should work, however it will most definitely give precedence to cases that we may not want to prioritize (e.g. DGX-1 not being ELECTRONIC is probably more acceptable than 2-Liter not being MEASURE). On the other hand, leaving the current setup with the limitations imposed by SP's limitations lets you keep a tight lid on what "electronic" things go in it by simply editing the .tsv file.

Perhaps, the best of two worlds would be to leave the above in place, and expand Whitelist to match everything ABC-### and ###-ABC? That way all electronics stay Electronicand we have aWhitelistthat capturesG-9, COVID-19`, etc.

[tbartley94]

So to reiterate: we add the functionality to both whitelist and electronic, but whitelist is more general while elctronic captures specific whitelist?

I think that can work. But we can also just add negative weights within whitelist to take care of precedence cases.

[zoobereq]

As extra as that sounds -- yes. It may not matter for the final output, but I think it would be useful to separate evidently ELECTRONIC things from WHITELIST during tagging.

If you don't think it matters all that much (and it may not), I'll add the changes to WHITELIST and we push like that.

[tbartley94]

Mmm, redundancy becomes a pain on debugging. Just move everything to whitelist, but make the key electronic terms in the actual whitelist while the more liberal expansion a few lines in the tagger graph. Make the weight for the whitelist file have highest priority. If we get annoyed by the liberal expansion, we comment out and work on later. How does that sound?

[zoobereq]

Sounds very sensible. Let me implement that and see how well it meshes with the rest of the the grammar.

[tbartley94]

Mmm, thoughts on doing a more comprehensive pattern? Technically we can extend to any use of [0-9]+-[A-Za-z] Can you see potential conflict with that?

[tbartley94]

Also reverse, would allow capturing things like G-9 summit, COVID-19.

[zoobereq]

Well, this will make it so that Electronic captures everything that's ###-ABC or ABC-###. This of course won't matter if all this is moved to Whitelist. Besides that, I don't think I'm seeing any immediate problems, which doesn't mean that they won't emerge once this is implemented.

[tbartley94]

Mmm, my vote is trying out but make sure it's a change that can be commented out. That way the fix can be quick if we break anything.

[zoobereq]

I added the explicitly targeted strings to WHITELIST. It turns out that the pattern [0-9]+-[A-Za-z] and its reverse is already matched by the MEASURE graph, so strings such as COVID-19 or G-9 will be tagged and verbalized accordingly. Since it's already there, I didn't reproduce it b/c redundancy.

[tbartley94]

Oh go figure. Can you do a quick check to see if that's consistent across the rest of the repo?

Will merge once import checks are fixed.

The merge-base changed after approval.

[tbartley94]

LGTM

[tbartley94]

lgtm

The merge-base changed after approval.

[tbartley94]

@zoobereq since i tried to push last time I'm not allowed for final review. Ping Marianna for a quick stamp.

The merge-base changed after approval.

The merge-base changed after approval.

The merge-base changed after approval.