First pass for ARHMM

[fausto-mpj]

I have not changed the sampling function yet, and the results are not checked for correctness. In the version, I just made sure that the changes you first implemented are running without any explicit error.

[gdalle]

Thank you @fausto-mpj for being willing to contribute! I'm excited to work on this with you.
I didn't look at the content yet (the tests are failing anyway) but I started with some advice on good PR practices. Since I don't know how experienced you are with Julia and GitHub, I made very broad remarks, so don't take them personally :)

[fausto-mpj]

I'm finishing the second pass today (probably), but this time I will run the tests before re-submitting the PR. I have some questions about the code and I will submit them together with the PR.

(Some of them are already written as comments in my last commit, but I will organize them later to make it easier to review!)

[gdalle]

Awesome, thank you!

[fausto-mpj]

I'm having some issues with (1) the allocation tests, (2) Baum-Welch convergence, and (3) the autodiff test.

With respect to (1), I believe it is due to changes in the _params_and_loglikelihoods function in chainrules.jl (line 11), but I'm not sure. I don't remember changing anything that would allocate instead of using your in-place operations.

Now, (2) and (3) I believe are correlated and both mostly affect autodiff.jl. I barely changed the files for the examples... I just adapted them to the changes in the inference code, but now they aren't working properly according to the tests.

The changes in inference were mostly to adapt for the presence of control and prev_obs, except for some parts where I changed the index of the control variable in the transition_matrix function. The reason for these are in forward.jl, line 106. I'm not sure if I understood correctly, but if the transition matrix is from t to t+1, then some indices were off, as the index of the control variable should match the next step ( t+1): $P(X_{t+1} \vert X_{t}, U_{t+1})$.

I will keep trying tomorrow, but a second pair of eyes would be wonderful!

[gdalle]

Thank you for this, and sorry for the delay. I'll try to take a look this week.

[fausto-mpj]

I've also finished the $n$-best Viterbi (aka List Viterbi) in a new file (list_viterbi.jl), with a little modification in the linalg.jl (I've only added an argmaxplus_transmul! function with an additional parameter to rank the $n$ best candidates at each t). I tried my best to make it match your code style in viterbi.jl.

The reference paper is here.

The reference implementation (in Python) is here. I've used this as a "ground truth" to test the results of list_viterbi.jl.

Do you want me to commit these changes to this branch, or make a different PR?

[gdalle]

Do you want me to commit these changes to this branch, or make a different PR?

I think these should definitely go in a separate PR (and that PR will be much easier to review too)

[fausto-mpj]

Got it! As soon as we finished this one, I will adjust whatever is necessary and submit the List Viterbi in a different PR.

[codecov]

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.95%. Comparing base (fc44a53) to head (4c1b890).

Files with missing lines	Patch %	Lines
src/types/abstract_hmm.jl	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            gd/ar     #123      +/-   ##
==========================================
- Coverage   95.27%   94.95%   -0.33%     
==========================================
  Files          18       18              
  Lines         508      515       +7     
==========================================
+ Hits          484      489       +5     
- Misses         24       26       +2     

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

[gdalle]

Thanks for working on this! As a general rule, you probably want to disrupt existing code as little as possible when you make a contribution, and abide by the interfaces that are already there. In this instance, this implies

• using the same convention for time indices
• avoiding overloads for Base functions like size which do something unexpected