-
Notifications
You must be signed in to change notification settings - Fork 14
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
Use numerically stable one-pass algorithm for log-sum-exp #18
Conversation
Looks nice @devmotion, thanks for contributing this. I'm rebasing to the numeric branch, which has been long running, as I intend to merge it back to master soon. That will also run the full test suite, specifically so we can confirm that the log-normalizing constants for the various examples are consistent. For tests, you can add them to Finally, is there a reference to explain the numerical stability aspect of this to a future reader? The blog post linked in the code comments has a nice explanation of the streaming part, but not the numerical stability part, which I think you may have added. |
Normalizing constant tests are failing, e.g. These tests are new on branch numeric, and don't exist on master, so this was not caught until now. They're all those beginning with Note that it's the |
Thanks!
OK, I'll add tests when I'm back from vacation next week.
True, I added that in the Julia implementation but it was not part of the blog post. The idea is to only add the |
Oh, I'll check what's the problem there and try to fix it. I wonder if it is caused by the NaN values in |
Codecov Report
@@ Coverage Diff @@
## numeric #18 +/- ##
===========================================
+ Coverage 81.24% 81.27% +0.03%
===========================================
Files 446 447 +1
Lines 18272 18354 +82
===========================================
+ Hits 14845 14918 +73
- Misses 3427 3436 +9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Thanks for the updates on this @devmotion. I made a few minor tweaks for style (spaces around low precedence operators, rewrote a couple of nested |
Also, once you merge updates on |
Sorry for the slow progress here, I was quite busy last week. I added some explanations and tests (revealed an incorrect result of Indeed, the |
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.
Thanks @devmotion. Looks solid, only issue is program
name, see comments. Otherwise, I left a few comments mainly style related, but keen to keep a consistent style throughout the code.
* Test log-sum-exp implementations in `log_sum_exp` and | ||
* `resample_reduce`. | ||
*/ | ||
program test_logsumexp() { |
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.
Should be test_basic_logsumexp
to be picked up and run by the grep
in smoke.sh
and test.sh
. Or I'd even suggest test_basic_log_sum_exp
here to be consistent with the name of the function.
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.
Similar throughout, logsumexp
-> log_sum_exp
.
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.
Ah I had noticed this problem as well and thought I had pushed a commit that fixes the name. It seems I did not 😄
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.
Oh, actually I think I renamed it to test_basic_logsumexp
in bc6d13a, so maybe you were looking at an older commit? In any case, I'll rename logsumexp
to log_sum_exp
.
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.
I renamed all occurrences of logsumexp
(also in the filename) to log_sum_exp
.
} | ||
|
||
// Special cases involving -inf, inf, and nan. | ||
let cases <- [ |
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.
Nice! Thanks for including all these edge cases.
No worries. Just favoring the neater code, and can expect the compiler to optimize it out here. |
Thanks for all the revisions @devmotion. Looks good. I've merged back to |
A while ago I wrote a Julia implementation of a numerically stable one-pass algorithm for log-sum-exp (original PR: JuliaStats/StatsFuns.jl#97, later extracted to https://github.com/JuliaStats/LogExpFunctions.jl/blob/master/src/logsumexp.jl). This one-pass algorithm is faster (at least for the inputs I benchmarked) and also fixes some underflow issues of the standard implementation of log-sum-exp (that are present in Birch as well). Hence I thought it might be useful for Birch as well and switched
log_sum_exp
andresample_reduce
in the standard library to the one-pass algorithm.I checked the implementation with the following Birch code: https://gist.github.com/devmotion/a6c3561f6c593160744147e7c5165f62
The output indicates that the PR fixes the underflow issue and improves performance:
I guess it might be good to add the underflow example and possibly some other tests. Where should they be added?