Experimental hmm

[jeromekelleher]

Just opening for visibility and discussion here, not for merging. I've based the changes on 0.3.3 for simplicity here as sc2ts isn't compatible with some of the internal changes in tsinfer (yet). I'll cherry pick these changes onto another PR if we decide to implement this for real (probably adding an option to rescale or not by n)

A quick implementation of the changes discussed in jeromekelleher/sc2ts#242 where we want to simplify the HMM and make it easier to reason about which Viterbi paths we care about.

[jeromekelleher]

Note: the only test that's failing is one that's poking into the details of parameter values. Everything else is passing fine.

[jeromekelleher]

Some quick notes here on using c11 atomics - getting things to compile on Unix platforms is easy, but Windows is problematic. It seems that MSVC doesn't support stdatomic.h, so I've hacked around it by just not making these variable in question an atomic. In the short term this is fine as we're only using this in sc2ts and this doesn't support Windows. It would raise some questions about whether putting this into tskit is worth the hassle, though.

[hyanwong]

I don't know anything about atomics, but https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/ suggests that stdatomic.h is coming to windows, and can be enabled in the current MSVC using the /experimental:c11atomics switch?

[jeromekelleher]

Good to know. I guess that'll filter down to Python builds sometime around 3.15 or so (Python builds against specific MSVC, 14.x here for what we're working with).

[codecov]

Codecov Report

Attention: Patch coverage is 80.86957% with 22 lines in your changes missing coverage. Please review.

Project coverage is 93.26%. Comparing base (8bd218d) to head (7522bcc).

Files with missing lines	Patch %	Lines
lib/err.c	75.36%	9 Missing and 8 partials ⚠️
lib/tree_sequence_builder.c	78.57%	1 Missing and 2 partials ⚠️
tsinfer/inference.py	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           0.3.3-backport     #959      +/-   ##
==================================================
- Coverage           93.56%   93.26%   -0.30%     
==================================================
  Files                  17       17              
  Lines                5546     5628      +82     
  Branches             1007      987      -20     
==================================================
+ Hits                 5189     5249      +60     
- Misses                235      248      +13     
- Partials              122      131       +9     

Flag	Coverage Δ
C	93.26% <80.86%> (-0.30%)	⬇️
python	96.57% <80.00%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jeromekelleher]

@hyanwong @szhan @benjeffery - one thing to note here is that the last couple of commits remove the low-level requirement that the ancestral state must be 0. So we should definitely port this much into the mainline tsinfer at some point, as it has been a persistent headache.

Perhaps worth doing for the next release, as it may simplify some aspects of the VCF Zarr processing?

[benjeffery]

Very nice! I assume you mean just bringing across only the code that removes the requirement?