Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add onepass algorithm for logsumexp #97
Add onepass algorithm for logsumexp #97
Changes from 8 commits
52a63ed
5983de9
c9e1396
70c1b88
31356bd
44eec25
51ca6db
7937e09
b79aceb
621c4f6
d0aa84a
0dff349
70995c5
09bd293
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 guess it's also required by
Array
when the input is empty, right?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.
No, empty inputs are handled by
log(sum(X))
, as before. Actually we never want to use this method apart for GPU arrays - the type calculations are doomed to fail e.g. for not concretely typed arrays and hence it is much safer to not rely on them and instead just compute the output. I'm not sure why the type calculations in https://github.com/JuliaGPU/GPUArrays.jl/blob/356c5fe3f83f76f8139b4edf5536cbd08d48da7f/src/host/mapreduce.jl#L49-L52 fail, I can check if this can be fixed from our side. Alternatively, maybe another package such as NNlib should provide a GPU-compatible overload of StatsFuns.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.
OK. They fail because
neutral_element
is only defined for a few particular functions. Another approach would be to use the type of the first element, since we already handle the case where the array is empty. That way we wouldn't need two different methods depending on whether the eltype is known.Given that it seems doable, it would be nice to have something that works for all arrays without another package having to overload the method.
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.
IMO it is just bad that GPUArrays requires us to provide an initial or neutral element. We really never want to do that, regardless if there is a first element or not. Retrieving the first element is bad for any stateful iterator, and is a scalar indexing operation on the GPU (which leads to warnings when calling the function - and we can not remove them without depending on CUDA). Additionally, the docstring of
mapreduce
says thatso there is really no point in providing
init
here - apart from GPU compatibility. The problem is that we can not restrict our implementation with theinit
hack to only GPU arrays without taking a dependency on CUDA. Hence I don't think it is completely unreasonable to provide a GPU-compatible implementation somewhere else. An alternative would be to not use the one-pass algorithm as the default algorithm (for arrays).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.
Retrieving the first element should be OK for any
AbstractArray
(contrary to some iterables). It's too bad that CUDA prints warnings in that case.Let's keep what you have now: since the method only accepts arrays with
T<:Real
element type,float(T)
will (almost) certainly work.