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

Clarify some comments in rm_epsilon; fix possible bug there. #965

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

danpovey
Copy link
Collaborator

No description provided.

@danpovey
Copy link
Collaborator Author

RE that change in rm_epsilon.cu: that loop would have looped indefinitely if those 2 arrays were different after the 1st iter.
So it looks to me like maybe the epsilon removal algorithm was never tested for inputs with 2 successive epsilons??? My bad, I suppose, but it would be nice if someone e.g. @pkufool could fix this.

@pkufool
Copy link
Collaborator

pkufool commented Apr 25, 2022

but it would be nice if someone e.g. @pkufool could fix this.

Sure.

@@ -767,9 +767,9 @@ void ComputeEpsilonClosure(FsaVec &epsilon_fsa, FsaVec *closure_fsa,
// We repeatedly call ComputeEpsilonClosureOneIter() until there is no further
// change in the FsaVec (this can be by simple comparison on arcs vector,
// since thanks to sorting the order is deterministic).
Array1<Arc> epsilon_fsa_arcs = epsilon_fsa.values;
Array1<Arc> &epsilon_fsa_arcs = epsilon_fsa.values;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, sorry, I realized this is not the source of the problem, I did not notice the statement below, closure_fsa_arcs = cur_iter_closure_fsa.values;
I will refactor a little for clarity though.

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