Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decompose forward function into initialize, predict, update #105

Merged
merged 5 commits into from
Oct 2, 2024

Conversation

THargreaves
Copy link
Contributor

As discussed in #104, this PR takes the existing forward filtering code and decomposes it into separate functions for initialising the filter, predicting the next state and updating the filtered state.

There are a few mildly ugly corners:

  • logL is passed into _update! and returns. This is because the logL in storage is not updated until the full forward filtering is completed. This could be resolved by passing a k argument into forward! and updating logL[k] in-place but changing this signature had knock on effects on other algorithms (e.g. Baum-Welch) so I've left for now
  • The prediction step is skipped for t=1 since we just use the initialisation value.

On the bright side, this decomposition has made the indexing a bit easier to understand. The loop is now t1:t2 and the main index used is t rather than t+1 before.

All correctness tests pass but there are few test cases in the AD/temporal examples that are failing. Happy to look into these but @gdalle might be able to instantly spot what's wrong.

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.22%. Comparing base (38eb996) to head (6b2a3d4).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   95.20%   95.22%   +0.02%     
==========================================
  Files          17       18       +1     
  Lines         500      503       +3     
==========================================
+ Hits          476      479       +3     
  Misses         24       24              

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

@gdalle
Copy link
Owner

gdalle commented Oct 1, 2024

@THargreaves what do you think? Are these the functions you would need to be part of the public API?

@THargreaves
Copy link
Contributor Author

Yes, those are perfect. Thank you for making the changes!

I'm not sure if they necessarily need to be part of the public API as we'll be wrapping them to match our slightly different type signature. If that's the easiest way to make sure these methods don't change though that might be best.

@gdalle
Copy link
Owner

gdalle commented Oct 2, 2024

Indeed if it's not part of the public API then it is vulnerable to change at any second. My suggestion is that I'll first release a new version of HMMs including them as private API, that way you can experiment and see what's missing. Once your code stabilizes, we'll figure out the best way to make its ingredients public API in HMMs.

@THargreaves
Copy link
Contributor Author

Sounds like a plan 👍

@gdalle gdalle merged commit 1656f7d into gdalle:main Oct 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants