Refactoring HMM #125
Refactoring HMM #125
Conversation
HMM -> HiddenMarkovModel Internal_HMM -> HMM CHMM_Base -> ConstrainedHiddenMarkovModel
1. Add flag for error checking in HMM 2. Broke apart the a_star and viterbi methods into separate files
Demonstrating tests using the Oracle HMM interface
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
- Coverage 85.12% 83.10% -2.02%
==========================================
Files 83 84 +1
Lines 5390 5458 +68
Branches 575 594 +19
==========================================
- Hits 4588 4536 -52
- Misses 688 795 +107
- Partials 114 127 +13 ☔ View full report in Codecov by Sentry. |
The hmm/inference classes capture this functionality now.
Or 'observed states'
There was a problem hiding this comment.
I like the idea a lot! I don't know how I feel about the naming convention though. It was hard in my head to keep track of HMM vs. HiddenMarkovModel. Also, for things like Bayes Nets, it would be less obvious what the acronym vs. non-acronym version would be.
Maybe HMM and _HMM? Or Matrix_HMM or something? It's not obvious to me what's best.
|
@clmatt I agree that the naming convention HiddenMarkovModel/HMM isn't intrinsically better. I'm trying to have a consistent camel-case convention for end-user classes (e.g. MarkovNetwork, DiscreteBayesianNetwork). I like the idea of a more explicit class name than HMM. It's clear now that HMM is only used to drive a few specific inference methods. For example, it isn't being used for the LP/IP inference methods. Hence, I don't think we can/should promote it as a general "internal" representation for all inference methods. Similarly, I think I'm going to shift the code to not automatically generate this representation (since it isn't always used). I'm leaning towards your recommendation. How about HMM_Matrix? Or HMM_MatVec? What about instance naming convention? Right now the HiddenMarkovModel has an hmm property for the HMM_Matrix instance. Maybe we should call that hmm_matvec to be more explicit? Or something else? Maybe use a convention like hmm_ to reference internal matrix representations? |
The use of the ConstrainedHiddenMarkovModel is now done underneath the hood of this method. Thus, lp_inference accepts a HiddenMarkovModel, and ip_inference accepts a ConstrainedHiddenMarkovModel
|
FYI, I've started reworking the scripts in clio_collab to confirm that this refactoring is not disruptive. The main changes were w.r.t. the use of the new *Inference() classes to execute map_queries, rather than directly calling inference functions. Otherwise, minimal changes are needed to account for this refactor (and other conin changes since the move from clio)! |
2 participants
Changes to create an HMM API that uses composition rather than inheritance.