Add HSMM type hierarchy and simulation - #164
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
==========================================
+ Coverage 95.79% 96.30% +0.50%
==========================================
Files 20 22 +2
Lines 618 676 +58
==========================================
+ Hits 592 651 +59
+ Misses 26 25 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gdalle
left a comment
There was a problem hiding this comment.
Thanks, this looks good! A few minor nitpicks
|
gentle bump on this |
|
|
||
| Simulate `hmm` for `T` time steps, or when the sequence `control_seq` is applied. | ||
|
|
||
| Return a named tuple `(; state_seq, obs_seq)`. |
There was a problem hiding this comment.
Removing this guarantee is technically breaking? It does still hold for AbstractHMM, doesn't it?
There was a problem hiding this comment.
yes this is still accurate, I have it in a comment in the AbstractHMM specific rand. I'll re-add to docstring as that's important info
|
|
||
| Return the matrix of state transition probabilities for `model` (possibly when `control` is applied). | ||
|
|
||
| For an [`AbstractHSMM`](@ref) the diagonal of this matrix must be zero (no self-transitions). |
There was a problem hiding this comment.
How can this be true in general since AbstractHMM <: AbstractHSMM?
There was a problem hiding this comment.
you're right -- i'll reword
| log_transition_matrix(hmm::AbstractHMM, ::Nothing) = log_transition_matrix(hmm) | ||
| obs_distributions(hmm::AbstractHMM, ::Nothing) = obs_distributions(hmm) | ||
| # No-op for duration distributions, since HMMs don't have them. | ||
| duration_logdensity_type(::AbstractHMM, control) = Union{} |
There was a problem hiding this comment.
Shouldn't we rather implement duration_distributions for AbstractHMM? Since it is a subtype, it would makes sense for it to adhere to the API requirements of its supertype
There was a problem hiding this comment.
I mulled on this. In theory we could --- HMMs implicitly have a Geometric sojourn distribution, and we can convert an HSMM to its equivalent HMM without the explicit distributions and vice versa. A few reasons I didn't immediately do this:
- The point of the subtyping was to eliminate code duplication, so I wanted any effect on
HMMinference / learning to be zero-cost, or, for a good enough reason, extremely minimal. TheUnion{}return here does that: sincepromote_type(T, Union{}) === T, the duration term drops out of theeltypepromotion andHMMforward / forward_backward are unchanged. - I wanted to avoid a dep. The only distribution that makes sense here is
Geometric, so we'd either have to roll our own (which I'd honestly prefer) or pull inDistributions.jljust for it. - We'd need to add machinery in forward / forward_backward to keep the underlying
duration_distributionsupdated when they're present; probablyconvert_transitionat every iteration that finds the equivalent set ofGeometricdists. - For someone reaching for a plain
HMM, it's hard for me to envisionduration_distributionsbeing something they'd actually want or need.
So I guess tl;dr: I did not implement it this way because I saw it as minimally helpful for the additional code and possibility of introduced bugs
One thing we could consider, and I think could be worth having, is a small set of utilities: from_hmm / from_hsmm so that if you learn an HSMM with Geometric durations you can convert it to the equivalent HMM and vice versa. Could be handy for model comparison, where you want the parameter counts to line up correctly. This doesn't really address the broader point about not adhering to the subtype interface but could be an alternative
| # One duration distribution per state. | ||
| length(durations) == N || return false | ||
|
|
||
| # No self-transitions (allowing for numerical noise). |
|
@gdalle can I get a ruling on something before I continue? In this PR I've made it so that The issue is best seen in the transition matrix and the duration distributions (though violating the duration-dist semantics is a bit more defensible, I think), as you flagged. An It's mathematically defensible, but the zero-diagonal contract doesn't literally hold for the subtype and I think the real reason is that the two transition matrices aren't the same object. For an So I see a couple of ways forward and I'm curious which you'd prefer:
|
|
I would probably go for solution two, after reading your explanation that's what I wanted to do as well |
|
gentle bump on this |
|
I'll be at JuliaCon this week but I'll try to take a look, sorry! |
|
oh right that's this week--enjoy the conference! This can wait |
Second step in porting Hidden Semi-Markov Models into the package. This PR adds the
HSMMtype layer and sampling only. Unifies the abstract-type hierarchy soAbstractHMM <: AbstractHSMM, introduces the concreteHSMMtype, and implements duration-awarerand.