fix: remove lower and nfc_normalization from default cleaners #482
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joanise
reviewed
Jun 21, 2024
everyvoice/text/text_processor.py
Outdated
Comment on lines
422
to
423
| ) -> list[str]: | ||
| ... |
There was a problem hiding this comment.
This is black 23 formatting, please re-install requirements.dev.txt to get black 24 and revert this change.
everyvoice/wizard/dataset.py
Outdated
Comment on lines
432
to
436
| self.state[ | ||
| "model_target_training_text_representation" | ||
| ] = apply_automatic_text_conversions( | ||
| self.state["filelist_data"], | ||
| self.state[StepNames.filelist_text_representation_step], |
everyvoice/wizard/dataset.py
Outdated
Comment on lines
579
to
584
| self.state[ | ||
| "model_target_training_text_representation" | ||
| ] = apply_automatic_text_conversions( | ||
| self.state["filelist_data"], | ||
| self.state[StepNames.filelist_text_representation_step], | ||
| global_isocode=isocode, |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev.ap/data-attestation #482 +/- ##
===========================================================
+ Coverage 73.96% 73.98% +0.02%
===========================================================
Files 45 45
Lines 2865 2868 +3
Branches 444 445 +1
===========================================================
+ Hits 2119 2122 +3
Misses 661 661
Partials 85 85 ☔ View full report in Codecov by Sentry. |
this should be fixed at a later date by fixing #359
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #321
PR Goal?
This removes lowercasing and NFC normalization as default cleaners and allows them instead to be specified in the wizard as expected given the wizard's question.
Fixes?
#321
Feedback sought?
try it out. I think as discussed with @MENGZHEGENG we should actually allow for dataset-specific cleaners instead of these global cleaners. In this PR, if someone selects an NFC cleaner for one dataset, it will get added to the global cleaners list, and apply to all datasets, which isn't correct (see #359). However, this is better than applying lowercasing and NFC normalization to every dataset regardless of what the user selects in the wizard. Let me know if you agree. I think the quick alternative would be to both remove
lowerfrom the default cleaner and remove the text processing question altogether until we fix this issue.Priority?
alpha
Tests added?
adjusted unittests to fit new paradigm
How to test?
the SENCOTEN data should work now for example
Confidence?
medium
Version change?
normally yes, but N/A because pre-alpha
Related PRs?