Skip to content
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

Connect word ends to initial state for context-independent allophones #50

Merged
merged 3 commits into from
Nov 24, 2023

Conversation

SimBe195
Copy link
Collaborator

For context-independent allophones (i.e. history_length = future_length = 0) the word-end states were not connected to the initial state in the ClassicTransducerBuilder which results in a broken automaton and a crash during training with fast_bw loss (e. g. CTC).
A fix for this behaviour had already been implemented in the original label_sync_decoding branch of @ZhouW321 but was missed when porting to github RASR. This PR re-applies the original fix.

@SimBe195 SimBe195 requested a review from curufinwe April 14, 2023 12:47
@mmz33
Copy link
Member

mmz33 commented Apr 18, 2023

Hey @curufinwe, we need this urgently for some experiments so could you please review this soon if possible? thank you.

@curufinwe curufinwe requested a review from Marvin84 July 11, 2023 12:50
Copy link
Contributor

@curufinwe curufinwe left a comment

Choose a reason for hiding this comment

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

I'm currently not super familiar with this part of the code. Could you attach plots of a simple automaton before and after your fix? Thanks!

src/Am/ClassicTransducerBuilder.cc Outdated Show resolved Hide resolved
@Marvin84
Copy link
Contributor

Marvin84 commented Jul 11, 2023

Yes, me and @michelwi are aware of this problem, since the FSA that uses allophones without right and left history is broken. I agree that there should be more documentation and test that confirms the correctness of the resulting FSA and I also suggest that this PR includes SVGs of an example.

I personally find that this solution is not mathematically incorrect. The classic FST creation is constructing an HCLG graph, and the developer of seq2seq framework needs an HLG graph, but by making all right and left context to be equal to # you are not creating an HLG graph. The correct solution should have been skipping the C composition.

@SimBe195
Copy link
Collaborator Author

SimBe195 commented Aug 2, 2023

I'm currently not super familiar with this part of the code. Could you attach plots of a simple automaton before and after your fix? Thanks!

@curufinwe Without the fix the words in a segment are not connected which causes the allophoneStateToPhonemeTransducer to not have a final state, so after trimming the automaton will be empty (and further processing steps crash).
For visualization I have selected a short segment containing only the words "THIS WEEK" and I have removed the Fsa::trim so that you can see the automaton properly.

This is the allophoneStateToPhonemeTransducer without the fix:
graphviz

And this is the allophoneStateToPhonemeTransducer with the fix:
graphviz(1)

Co-authored-by: Eugen Beck <[email protected]>
@albertz
Copy link
Member

albertz commented Nov 9, 2023

Note that this fix here is needed to fix #68 and rwth-i6/returnn#1456.

@Marvin84
Copy link
Contributor

Regarding the allophoneStateToPhonemeTransducer I am attaching here the resulting FSA from our standard HMM with the proper composition. As you can see, there are no dead-end nodes, meaning all paths will finish in one final node. I believe this is one first trivial condition for a proper FSA definition. The question is how this current composition affects the optimization and convergence when for example doing full-sum training.

allophon

@curufinwe curufinwe merged commit eb769ca into master Nov 24, 2023
@curufinwe curufinwe deleted the PhonologyFix branch November 24, 2023 10:26
SimBe195 added a commit that referenced this pull request Jul 31, 2024
Marvin84 pushed a commit that referenced this pull request Oct 9, 2024
Marvin84 pushed a commit that referenced this pull request Oct 10, 2024
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.

5 participants