-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dev.ap/text pf updates expanded #627
base: main
Are you sure you want to change the base?
Conversation
Changed Files
|
whoops - looks like some tests aren't passing - I'll investigate, but it should still be ready for review |
@joanise - I don't understand why the doctests are failling here, they're not failing on my machine, do you have any ideas? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #627 +/- ##
==========================================
- Coverage 76.24% 76.21% -0.04%
==========================================
Files 47 47
Lines 3490 3536 +46
Branches 481 493 +12
==========================================
+ Hits 2661 2695 +34
- Misses 726 734 +8
- Partials 103 107 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We didn't publish 0.3.0 yet, so this PR should update the 0.3 schemas, not create the 0.4 schemas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, a bunch of comments suggesting changes.
punctuation_features.append([1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["question_symbols"]: | ||
punctuation_features.append([0, 1, 0, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["big_breaks"]: | ||
punctuation_features.append([0, 0, 1, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["small_breaks"]: | ||
punctuation_features.append([0, 0, 0, 1, 0, 0, 0, 0]) | ||
punctuation_features.append([0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["periods"]: | ||
punctuation_features.append([0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["colons"]: | ||
punctuation_features.append([0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["semi_colons"]: | ||
punctuation_features.append([0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["commas"]: | ||
punctuation_features.append([0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["hyphens"]: | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["quotemarks"]: | ||
punctuation_features.append([0, 0, 0, 0, 1, 0, 0, 0]) | ||
elif char == self.punctuation_hash["ellipsis"]: | ||
punctuation_features.append([0, 0, 0, 0, 0, 1, 0, 0]) | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0]) | ||
elif char == self.punctuation_hash["parentheses"]: | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0]) | ||
elif char == self.punctuation_hash["ellipses"]: | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0]) | ||
elif char == self.punctuation_hash["exclamations"]: | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 1, 0]) | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0]) | ||
elif char in self.config.symbols.silence: | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 0, 1]) | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]) | ||
else: | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 0, 0]) | ||
punctuation_features.append([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not a fan of this long if/elif list. Can we not construct a dict from punc hash value to these one-hot vectors with a loop, and do a dict look up for the punc entries, followed by the last elif and else?
I mean, this code works, so you don't have to change it, but it would be nicer to refactor it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I kind of like just being able to see the full embedding like this - let's talk at the meeting today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't been able to test yet, just tiny comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR breaks fs2 and hfgl models, it will need to include targeted messages when a user tries to load a model created with an older version of EV that's not compatible with the current punctuation lists.
includes features for stress and special tokens
since panphon already deals with them also adds parentheses support in pf encoding
cbcf71a
to
78dc32c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this generally looks good, we're getting close to be able to merge. Just a couple small comments in the related PRs.
PR Goal?
This PR improves the phonological feature calculation by:
Fixes?
#357
Feedback sought?
Sanity + high-level comments. low-level code comments are also welcome but less of a priority.
Priority?
medium
Tests added?
I've added some tests and updated existing ones
How to test?
A little bit tricky to test, but maybe inspecting some of the doctests will help. The main work is just analysing the updated code in text/features.py
Confidence?
medium (I think this is an improvement, and it is what we use in PF-BERT, but it's a big set of interrelated problems so I would appreciate the feedback)
Version change?
Yes, this is a breaking change.
Related PRs?
EveryVoiceTTS/DeepForcedAligner#34
EveryVoiceTTS/FastSpeech2_lightning#109