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

quantile gives wrong gradient on Julia 1.6 #973

Closed
jgreener64 opened this issue Jul 31, 2023 · 5 comments
Closed

quantile gives wrong gradient on Julia 1.6 #973

jgreener64 opened this issue Jul 31, 2023 · 5 comments

Comments

@jgreener64
Copy link
Contributor

On Julia 1.9 and Enzyme main (6692fad) this works:

using Enzyme, Statistics
f(x) = sum(quantile([1.0, x], [0.5, 0.7]))
autodiff(Reverse, f, Active, Active(2.0))[1][1]
1.2

But on Julia 1.6.7 it gives the wrong answer:

0.7

autodiff(Forward, f, Duplicated(2.0, 1.0))[1] also gives 0.7.

A possibly simpler variant: sum(quantile([1.0, x], 0.7)) gives the correct 0.7 on both versions but sum(quantile([1.0, x], [0.7])) gives 0.7 on Julia 1.9 (correct) and 0.0 on Julia 1.6 (incorrect).

@wsmoses
Copy link
Member

wsmoses commented Aug 6, 2023

Looks like the primal computation itself got messed up:

julia> autodiff(Forward, f, Duplicated,Duplicated(2.0, 1.0))
(0.0, 0.0)

julia> f(x)=sum(quantile([1.0, x], [0.7]))^C

julia> x=1.0
1.0

julia> quantile([1.0, x], [0.7])
1-element Vector{Float64}:
 1.0

julia> f(2.0)
1.7

julia> f(x)=sum(quantile([1.0, x], [0.7]))
f (generic function with 1 method)

@wsmoses
Copy link
Member

wsmoses commented Aug 6, 2023

using Enzyme, Statistics

Enzyme.API.printall!(true)

@inline function myquantile(v::AbstractVector, p::Real; alpha::Real=1.0, beta::Real=alpha)
    n = length(v)
    
    m = alpha + p * (one(alpha) - alpha - beta)
    aleph = n*p + oftype(p, m)
    j = clamp(trunc(Int, aleph), 1, n-1)
    γ = clamp(aleph - j, 0, 1)

    if n == 1
        a = v[1]
        b = v[1]
    else
        a = v[j]
        b = v[j + 1]
    end
    
    return a + γ*(b-a)
end

function f(x)
    v = [1.0, x]
    return first(map(x->myquantile(v, x, alpha=1., beta=1.), [0.7]))
end

@show f(2.0)
@show autodiff(Forward, f, Duplicated,Duplicated(2.0, 1.0))

@wsmoses
Copy link
Member

wsmoses commented Aug 6, 2023

This turns out to be a bug in the attributor which changes a store double ... into a store double undef

https://godbolt.org/z/Pa7Eejcq4

@jdoerfert any insights here?

@wsmoses
Copy link
Member

wsmoses commented Aug 6, 2023

The bug appears to occur in LLVM 11 (above) and 12 (https://godbolt.org/z/YehKx37h8), but not 13+

@wsmoses
Copy link
Member

wsmoses commented Aug 7, 2023

This is effectively fixed by disabling the attributor in #986 for lower LLVM versions, but is still quite unsatisfying. Either way closing as now resolved and the attributor bug is independent of Enzyme.jl

@wsmoses wsmoses closed this as completed Aug 7, 2023
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

No branches or pull requests

2 participants