-
Notifications
You must be signed in to change notification settings - Fork 63
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
Free list transformer to replace deprecated ListT #253
base: master
Are you sure you want to change the base?
Conversation
I observed runtimes to go down by 30%, and never going up in comparison to Adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing! My one request would be an update to the documentation here: https://monad-bayes.netlify.app/usage/#population You can update this by changing the relevant markdown file in the docs. Just a quick explanation of the motivation of the new representation, where it comes from, maybe the definition too. I can also have a go at this, but would be nice to check if it's straightforward for you to update the docs.
Yes, on it.
Very good point. I have trouble understanding the current documentation:
Shouldn't it be:
|
newtype Population m a = Population (Weighted (ListT m) a)
|
Nice! |
Ah, true. I guess it's implemented as I wonder what we should do documentation-wise. I'd find it simpler to explain that |
We should understand what the proposed change is doing a bit more before merging. I wrote it in issue #245, but it also belongs here. The paper and its follow-up used the monad-structure transformer In general, free monads may do something different, for example, suspend random samples that some of the monad-bayes combinators do in crucial places. This might mean, for example, that using the same combinations of monad-bayes's building blocks to implement algorithms such as smc, smc^2 etc. are not longer correct. I suggest we first understand better how this proposed change affects the existing inference algorithms, not just in runtime, but in the actual samples they compute, both by testing and using an accompanying formalisation. |
f634a51
to
a20a435
Compare
That's right. I'm still surprised that there isn't a Writer implementation that is suitably efficient (or even just uses State under the hood), but I don't know enough about performance to solve that problem. (I tried the continuized Writer, and I recall seeing mild speedups or at least, no slowdowns, although I might be misremembering).
I agree in theory, but I think this would just cause extra confusion when anyone looked at the code and saw State, not Writer. |
7f7bf90
to
6493bed
Compare
I tried to go for a conservative approach now that keeps an One learning is definitely that using an unlawful monad is a maintenance nightmare. Replacing it with a lawful one and then trying to find all the places where the particular flattening behaviour was intended and where it was accidental is like looking for a needle in a haystack. If someone else wants to give this a go, I'm happy to hand over and advise, but I've done enough work on this now for free. Another thought I had while working on this issue: |
I also had that thought re. SequentialT, and attempted to replace with FreeT and a catamorphism, but discovered that my understanding was not good, because the "folding" in the coroutine operations was quite different to what I'd imagined. You might have more luck though. |
samples_free_listt.pdf
samples_master.pdf
Removes the dependency on the deprecated
ListT
in favour of a free monad construction. In fact improves benchmarks significantly!