Skip to content

Conversation

@YiwenShaoStephen
Copy link
Contributor

Add a new binary: chainbin/chain-make-num-fst-e2e
It is used to converts chain e2e numerator fst (containing transition-ids) to fst (containing pdf-ids,
zero-based, eps-removed, and normalized by the normalization fst).
This step is usually in prepare_egs stage while this binary would be necessary for other outside toolkits (e.g. pychain) that directly use fst instead of egs.

Copy link
Contributor

@danpovey danpovey left a comment

Choose a reason for hiding this comment

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

some comments..

@@ -0,0 +1,121 @@
// chainbin/chain-make-num-fst-e2e.cc

// Copyright 2019 Johns Hopkins University (author: Yiwen Shao)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just put Yiwen Shao as the author as you are not an employee (or if you have a part-time job with Hopkins, at least you are not doing this in that status)


namespace kaldi {

bool FstTransitionToPdf(const fst::StdVectorFst &fst_transition,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be .. ToPdfPlusOne(), since these are pdf+1. Also I think this conflicts with your comment or commit message in which you say they are zero-based, these seem to be one-based.

try {
const char *usage =
"Converts chain e2e numerator fst (containing transition-ids) to fst (containing pdf-ids, \n"
"zero-based, eps-removed, and normalized by the normalization fst) \n"
Copy link
Contributor

Choose a reason for hiding this comment

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

here is where it says zero-based. Also this doesn't remove epsilons, it just checks that there are no epsilons.. might be good to clarify. should say pdf-ids + 1.

@YiwenShaoStephen
Copy link
Contributor Author

Thanks for the comments! I've made modifications according to it.

@danpovey danpovey merged commit 88e7b85 into kaldi-asr:master Feb 26, 2020
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