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

Proposal: generalise logsumexp slightly. #69

Closed
wants to merge 1 commit into from
Closed

Proposal: generalise logsumexp slightly. #69

wants to merge 1 commit into from

Conversation

willtebbutt
Copy link

Will add thorough testing if this is deemed something that's reasonable to include. Provides functionality similar to that found in sum and maximum etc to provide a dims argument which acts in the same way as sum and maximum.

e.g.

B = randn(5, 4)
A = logsumexp(B; dims=1)

returns a 1 x 4 matrix A, where A[1, k] = logsumexp(B[:, k]).

@@ -190,7 +190,13 @@ function logsumexp(X)
isempty(X) && return log(sum(X))
reduce(logaddexp, X)
end
function logsumexp(X::AbstractArray{T}) where {T<:Real}
function logsumexp(X::AbstractArray{T}; dims=nothing) where {T<:Real}
dims isa Nothing && return _logsumexp(X)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more contentional to make this something like

Suggested change
dims isa Nothing && return _logsumexp(X)
dims === nothing && return _logsumexp(X)

though it doesn't really matter; they're entirely equivalent and should perform the same. Just figured I'd note it.

Copy link
Contributor

@cossio cossio May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnothing(dims)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=== is special cased in the compiler and is generally more efficient for checking nothing (and indeed, even missing).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think isnothing is also elided since it's dealt with at dispatch time, and it was the recommended way to check for nothingness? The only thing is that it requires Julia 1.1.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was the recommended way to check for nothingness?

How did you come to that conclusion?

I think isnothing is also elided

It is not, see JuliaLang/julia#27681.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you come to that conclusion?

Well, it's a exported function named isnothing :trollface:
In reality I wasn't aware of that issue. Thanks for pointing it out.

@ararslan
Copy link
Member

This certainly looks like a worthwhile feature to me!

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two minor comments.

function logsumexp(X::AbstractArray{T}) where {T<:Real}
function logsumexp(X::AbstractArray{T}; dims=nothing) where {T<:Real}
dims isa Nothing && return _logsumexp(X)
isempty(X) && return log(zero(T))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this type unstable? I think you'd still have to consider dimensions as in

julia> sum(zeros(0, 0), dims=1)
1×0 Array{Float64,2}

dims isa Nothing && return _logsumexp(X)
isempty(X) && return log(zero(T))
u = maximum(X; dims=dims)
return log.(sum(exp.(X .- u); dims=dims)) .+ u
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reuse the array created by sum(exp.(X .- u); dims=dims) to avoid a temporary.

@tpapp
Copy link
Contributor

tpapp commented Mar 20, 2019

Is this something that cannot be addressed with mapslices and similar?

@willtebbutt
Copy link
Author

Are there not performance issues associated with mapslices and related?

@andreasnoack
Copy link
Member

Are there not performance issues associated with mapslices and related?

Unfortunately, there are. At least there used to be. Could you benchmark it?

@willtebbutt
Copy link
Author

Here are some timings:

using BenchmarkTools, StatsFuns

N1, M1 = 1000, 2;
x = randn(N1, M1);
@btime logsumexp($x);
  20.012 μs (0 allocations: 0 bytes)
@btime logsumexp($x; dims=1);
  25.779 μs (13 allocations: 16.23 KiB)
@btime mapslices(logsumexp, $x; dims=1);
  25.318 μs (56 allocations: 10.53 KiB)
@btime logsumexp($x; dims=2);
  33.160 μs (31 allocations: 40.33 KiB)
@btime mapslices(logsumexp, $x; dims=2);
  401.447 μs (9507 allocations: 236.17 KiB)

# Second round of tests with different memory layout.
x2 = Matrix(x');
@btime logsumexp($x2);
  19.984 μs (0 allocations: 0 bytes)
@btime logsumexp($x2; dims=2);
  31.323 μs (31 allocations: 16.80 KiB)
@btime mapslices(logsumexp, $x2; dims=2);
  25.503 μs (56 allocations: 10.53 KiB)
@btime logsumexp($x2; dims=1);
  38.780 μs (13 allocations: 39.77 KiB)
@btime mapslices(logsumexp, $x2; dims=1);
  373.496 μs (9507 allocations: 236.17 KiB)

When each slice is small the mapslices overhead associated with mapslices is rather large.

@tpapp
Copy link
Contributor

tpapp commented Mar 20, 2019

Thanks for the benchmarks. This is because mapslices is poorly implemented. Cf

using BenchmarkTools, StatsFuns, JuliennedArrays

N1, M1 = 1000, 2;
x = randn(N1, M1);
@btime mapslices(logsumexp, $x; dims=2);
@btime map(logsumexp, Slices($x, False(), True()));

with

julia> @btime mapslices(logsumexp, $x; dims=2);
  704.918 μs (9507 allocations: 236.17 KiB)

julia> @btime map(logsumexp, Slices($x, False(), True()));
  52.297 μs (1003 allocations: 54.84 KiB)

I think it would be better to propagate the use of sane slice iterations constructs instead of adding a (; dims = ...) method to everything.

@willtebbutt
Copy link
Author

Closing for now as this has gone stale and a more generic mechanism of the form @tpapp suggests would probably better. Happy to re-open if anyone feels strongly about this.

@willtebbutt willtebbutt closed this Jun 6, 2019
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.

5 participants