Conversation
logdensityof
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #168 +/- ##
==========================================
+ Coverage 96.50% 97.06% +0.55%
==========================================
Files 24 25 +1
Lines 716 886 +170
==========================================
+ Hits 691 860 +169
- Misses 25 26 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if seq_ends isa NTuple{1} | ||
| for k in eachindex(seq_ends) | ||
| _forward!(storage, hsmm, obs_seq, control_seq, seq_ends, k; error_if_not_finite) | ||
| end | ||
| else | ||
| @threads for k in eachindex(seq_ends) | ||
| _forward!(storage, hsmm, obs_seq, control_seq, seq_ends, k; error_if_not_finite) | ||
| end | ||
| end |
There was a problem hiding this comment.
This could be shared via something like this:
| if seq_ends isa NTuple{1} | |
| for k in eachindex(seq_ends) | |
| _forward!(storage, hsmm, obs_seq, control_seq, seq_ends, k; error_if_not_finite) | |
| end | |
| else | |
| @threads for k in eachindex(seq_ends) | |
| _forward!(storage, hsmm, obs_seq, control_seq, seq_ends, k; error_if_not_finite) | |
| end | |
| end | |
| function foreach_sequence(kernel::F, seq_ends::AbstractVectorOrNTuple{Int}) where {F} | |
| if seq_ends isa NTuple{1} | |
| for k in eachindex(seq_ends) | |
| kernel(k) | |
| end | |
| else | |
| @threads for k in eachindex(seq_ends) | |
| kernel(k) | |
| end | |
| end | |
| return nothing | |
| end |
unclear if its worth the hassle or not
There was a problem hiding this comment.
The reason I put this workaround in place is because @threads was type-unstable, and I wanted a type-stable path at least in the single-sequence case. Not sure this is still necessary, and perhaps OhMyThreads.jl also has a better way to unify these cases
There was a problem hiding this comment.
Good point. I have switched @threads to use OhMyThreads.jl on StateSpaceDynamics.jl and it works very well. I'll redo this with that in mind then. Mayeb I'll do a separte PR for the HMM case and you can review before that
| $(TYPEDFIELDS) | ||
| """ | ||
| struct HSMMForwardStorage{R} | ||
| "posterior last state marginals `α[i] = ℙ(X[T]=i | Y[1:T])`" |
There was a problem hiding this comment.
i copied this form the equivalent struct for the HMM case. Would this be more accurate to say: "filtered state marginals α[i,t] = ℙ(X[t]=i | Y[1:t])". This somewhat to me implies only the last column
56d2596 to
6d6064e
Compare
e78bcc9 to
76ea431
Compare
Segment-based forward recursion for `AbstractHSMM`, with the same public interface as the HMM version: `forward` returns `(α, logL)` where `α[:, t]` are the filtered state marginals `ℙ(X[t]=i | Y[1:t])` on the probability scale, exactly as for an `AbstractHMM`. Two quantities are propagated per timestep. `log_ends[i, t]` is the mass of segments that end at `t` in state `i` and drives the recursion; `log_ongoing[i, t]` is the mass of segments that merely cover `t`, i.e. are right-censored there. Every prefix of a sequence ends mid-segment under the censoring convention of `rand`, so the likelihood and the state marginals are read from `log_ongoing` while the transition sweep consumes `log_ends`. Both are accumulated in the same duration sweep, so censoring costs no extra duration evaluations. `max_duration` is a keyword defaulting to the longest sequence length, which makes the recursion exact. Lowering it discards every segment longer than the cutoff, so the result is a strict lower bound that tightens as the cutoff grows, rather than an error of unpredictable sign. The observation logdensity prefix sums live in an `N × T` matrix matching the layout of `ForwardStorage.B`, and each sequence writes only inside its own columns, so threaded sequences never share an entry. The transition matrix is hoisted out of the segment loop when the controls are uniform, since for a model whose log-transitions are not cached, looking it up per timestep allocates a matrix per timestep. Tested by brute-force marginalization of `joint_logdensityof` over all state sequences, by normalization to one over all short observation sequences, and against the equivalent HMM under geometric sojourns, where both the loglikelihoods and the filtered marginals agree elementwise.
76ea431 to
912cc26
Compare
| cum_log_obs = Matrix{R}(undef, N, T) | ||
| obs_zeros = Matrix{Int}(undef, N, T) | ||
|
|
||
| # Per-sequence scratch space keeps parallel calls independent. |
There was a problem hiding this comment.
Back when I coded this package I handled this manually, but I think there are better ways now with OhMyThreads.jl
There was a problem hiding this comment.
| return reachable | ||
| end | ||
|
|
||
| function extend_segments!( |
There was a problem hiding this comment.
This has lots of positional arguments, the risk of a mixup is non-negligible
| if seq_ends isa NTuple{1} | ||
| for k in eachindex(seq_ends) | ||
| _forward!(storage, hsmm, obs_seq, control_seq, seq_ends, k; error_if_not_finite) | ||
| end | ||
| else | ||
| @threads for k in eachindex(seq_ends) | ||
| _forward!(storage, hsmm, obs_seq, control_seq, seq_ends, k; error_if_not_finite) | ||
| end | ||
| end |
There was a problem hiding this comment.
The reason I put this workaround in place is because @threads was type-unstable, and I wanted a type-stable path at least in the single-sequence case. Not sure this is still necessary, and perhaps OhMyThreads.jl also has a better way to unify these cases
| """ | ||
| $(SIGNATURES) | ||
| """ | ||
| function forward!( |
There was a problem hiding this comment.
Do you have a paper reference for this algorithm, so that I can try to follow the math?
There was a problem hiding this comment.
There was a problem hiding this comment.
We specifically are using the Explicit Duration HMM variant
There was a problem hiding this comment.
(we can reuse less code, but space complexity is far better)
|
I'm thinking of punting the There is also an issue with testing related to 1.13 where it seems the norm keyword is not being honored. I did not have this issue in 1.12.5. I did a quick |
|
Yeah, let's separate the threading updates from this new algo |
Julia 1.13 ignores the `norm` keyword of `isapprox` for arrays, so the coherence tests silently compared with the Frobenius norm instead of the intended infinity norm, making the tolerance stricter than intended. Replace the six call sites with an explicit `infapprox` helper.
|
just want to confirm this--I'm again not seeing a review/approval, but GH is letting me merge this, but I do not want to merge this prior to your go ahead. Can you confirm or deny an ongoing review on this? |
|
I haven't reviewed that one again and I would like to! |
Adds the segment-based forward algorithm for the HSMM mirrored as closely as possible to the HMM counterpart
forward follows the HMM interface and returns (α, logL). As with ForwardStorage.α, α[:, t] contains the filtered state marginals on the probability scale.
To support the right-censoring convention introduced in #167, the recursion tracks two quantities:
cc: @simonsteiger