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

Write logsumexp in a more generic form #45

Merged
merged 1 commit into from
Jun 26, 2018
Merged

Conversation

simonbyrne
Copy link
Member

This uses Julia's reduction functionality to improve the logsumexp
functionality. It no longer relies on 1-based indexing, is fast
on CuArrays, and there is now a generic fallback for iterable collections.

I've also renamed the 2-arg logsumexp to logaddexp, since Julia
convention is to use different functions for reductions and
reducers (e.g. max vs maximum).

This uses Julia's reduction functionality to improve the `logsumexp`
functionality. It no longer relies on 1-based indexing, is fast
on CuArrays, and there is now a generic fallback for iterable collections.

I've also renamed the 2-arg `logsumexp` to `logaddexp`, since Julia
convention is to use different functions for reductions and
reducers (e.g. `max` vs `maximum`).
@tpapp
Copy link
Contributor

tpapp commented Jun 6, 2018

Very nice work! I think that logaddexp makes a lot of sense. Please consider adding tests for all combinations of Inf, -Inf, NaN. Not because I doubt that your code is correct, but to ensure that all corner cases are covered in case of future changes to this code.

@simonbyrne
Copy link
Member Author

They were pretty exhaustively tested already (I was able to remove some of the branches since we fixed maximum to be NaN absorbing). Are there any other cases you think need adding?

@andreasnoack andreasnoack merged commit ae40bc1 into master Jun 26, 2018
@andreasnoack andreasnoack deleted the sb/logsumexp branch June 26, 2018 04:47
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.

3 participants