-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Fix bug identified in #1817, comment 17340049 #2524
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
Conversation
+1 for merge |
Looks good as well to me but I am not familiar with the HMM code. Maybe @kmike or @SnippyHolloW can have a look. If I understand correctly, observations with a sequence length of 1 are just ignored when updating the stats to estimate the transition probabilities? |
The bypass seems fine to me. I previously did the check on my side (before calling fit()). Maybe add a comment on the line of the "if > 1" to say there are no transitions to update with with 1 observation sequences? +1 for merge otherwise. |
+1 for adding an explicit inline comment. |
I was using end-of-sentence markers and haven't had this problem because of that. The PR looks good. |
comment applied. |
Thanks! merging. |
Fix bug identified in #1817, comment 17340049
Here's the bug report:
#1817 (comment)