Skip to content

[RFC/WIP] don't propagate NaN/Inf if Partials are zero #179

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.

Sign up for GitHub

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

Merged
jrevels merged 5 commits into from
Dec 26, 2016
Merged

[RFC/WIP] don't propagate NaN/Inf if Partials are zero #179

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
77e3736
don't propagate NaN/Inf if Partials are zero
jrevels Dec 15, 2016
5c51df7
attempt to fix Calculus.jl test failure on v0.4
jrevels Dec 22, 2016
b44a180
improve efficiency of NaN/Inf pruning by negating isfinite instead of…
jrevels Dec 23, 2016
0156387
add NaN-safe mode switch
jrevels Dec 26, 2016
b6cd27a
add documentation for NaN-safe mode
jrevels Dec 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ script:
- if [[ -a .git/shallow ]]; then git fetch --unshallow; fi
- if (julia -e 'VERSION < v"0.5" && exit(1)'); then
julia -e 'include(joinpath(JULIA_HOME, Base.DATAROOTDIR, "julia", "build_sysimg.jl")); build_sysimg(force=true)';
julia -e 'Pkg.clone(pwd()); Pkg.build("ForwardDiff"); Pkg.test("ForwardDiff"; coverage=true)';
julia -e 'Pkg.clone(pwd()); Pkg.build("ForwardDiff"); Pkg.checkout("DiffBase"); Pkg.test("ForwardDiff"; coverage=true)';
Copy link
Copy Markdown
Contributor

@tkelman tkelman Jan 25, 2017

Choose a reason for hiding this comment

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

Do you still need to do this? These kind of things should be very temporary, otherwise you're not testing against the same versions of things your users will have installed.

Copy link
Copy Markdown
Member Author

@jrevels jrevels Jan 25, 2017

Choose a reason for hiding this comment

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

DiffBase serves as a repository of test functions. If I didn't do this, I'd have to re-tag DiffBase every time I wanted to add a new shared test function, which would get really annoying.

I share your concern though. I've been meaning to factor the shared test suite out of DiffBase and into an unregistered package (DiffTestSuite or something). Then DiffTestSuite could be a test-only dependency that I Pkg.checkout every time, and I could go back to CI-testing DiffBase's functionality with the proper versions. That's what I probably should've done this in the first place...I'll set it up tomorrow.

Copy link
Copy Markdown
Contributor

@tkelman tkelman Jan 25, 2017

Choose a reason for hiding this comment

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

Pkg.add("ForwardDiff"); Pkg.test("ForwardDiff") should pass. If that requires tagging more often, tag more often. Pkg.test shouldn't depend on unregistered packages.

Copy link
Copy Markdown
Contributor

@tkelman tkelman Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Same dependency versioning issue applies as everywhere else - if it's needed to pass tests and doesn't exist in every version of DiffBase, then at least test/REQUIRE needs to have a corresponding minimum version requirement.

Copy link
Copy Markdown
Member Author

@jrevels jrevels Jan 25, 2017

Choose a reason for hiding this comment

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

Fair enough - I'll fix this to use tagged DiffBase versions and start tagging DiffBase more often, then.

Copy link
Copy Markdown
Contributor

@tkelman tkelman Jan 25, 2017

Choose a reason for hiding this comment

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

I don't mind if you want to run additional tests on CI-only, or have things temporarily checked out to branches on CI if that's necessary while things are being developed -- as long as you keep in mind how that configuration will differ from what users will have installed, and that Pkg.test should work for them on releases too.

Copy link
Copy Markdown
Member Author

@jrevels jrevels Jan 25, 2017

Choose a reason for hiding this comment

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

Yeah, no worries! My future workflow will be to keep master branch's CI running against tagged dependency versions for the reasons you mentioned. I can always toggle a PR's CI config to use whatever DiffBase branch I want during development, and then before merging, I can tag the new DiffBase version (if necessary) and switch the config back to using only tagged versions.

julia -O3 -e 'include(joinpath(Pkg.dir("ForwardDiff"), "test/SIMDTest.jl"))';
else
julia -e 'Pkg.clone(pwd()); Pkg.build("ForwardDiff"); Pkg.test("ForwardDiff"; coverage=true)';
julia -e 'Pkg.clone(pwd()); Pkg.build("ForwardDiff"); Pkg.checkout("DiffBase"); Pkg.test("ForwardDiff"; coverage=true)';
fi
after_success:
- julia -e 'cd(Pkg.dir("ForwardDiff")); Pkg.add("Coverage"); using Coverage; Coveralls.submit(Coveralls.process_folder())'
25 changes: 18 additions & 7 deletions src/partials.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,29 @@ Base.convert{N,T}(::Type{Partials{N,T}}, partials::Partials{N,T}) = partials
@inline @compat(Base.:+){N}(a::Partials{N}, b::Partials{N}) = Partials(add_tuples(a.values, b.values))
@inline @compat(Base.:-){N}(a::Partials{N}, b::Partials{N}) = Partials(sub_tuples(a.values, b.values))
@inline @compat(Base.:-)(partials::Partials) = Partials(minus_tuple(partials.values))
@inline @compat(Base.:*)(partials::Partials, x::Real) = Partials(scale_tuple(partials.values, x))
@inline @compat(Base.:*)(x::Real, partials::Partials) = partials*x
@inline @compat(Base.:/)(partials::Partials, x::Real) = Partials(div_tuple_by_scalar(partials.values, x))

@inline function _mul_partials{N}(a::Partials{N}, b::Partials{N}, afactor, bfactor)
return Partials(mul_tuples(a.values, b.values, afactor, bfactor))
# NaN/Inf-safe methods #
#----------------------#

@inline function @compat(Base.:*)(partials::Partials, x::Real)
x = ifelse((isnan(x) || isinf(x)) && iszero(partials), one(x), x)
Copy link
Copy Markdown
Contributor

@mlubin mlubin Dec 22, 2016

Choose a reason for hiding this comment

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

!isfinite could be better than isnan || isinf

return Partials(scale_tuple(partials.values, x))
end

@inline function @compat(Base.:/)(partials::Partials, x::Real)
x = ifelse(x == zero(x) && iszero(partials), one(x), x)
return Partials(div_tuple_by_scalar(partials.values, x))
end

@inline function _mul_partials{N}(a::Partials{N}, b::Partials{N}, x_a, x_b)
x_a = ifelse((isnan(x_a) || isinf(x_a)) && iszero(a), one(x_a), x_a)
x_b = ifelse((isnan(x_b) || isinf(x_b)) && iszero(b), one(x_b), x_b)
return Partials(mul_tuples(a.values, b.values, x_a, x_b))
end

@inline function _div_partials(a::Partials, b::Partials, aval, bval)
afactor = inv(bval)
bfactor = -aval/(bval*bval)
return _mul_partials(a, b, afactor, bfactor)
return _mul_partials(a, b, inv(bval), -(aval / (bval*bval)))
end

# edge cases where N == 0 #
Expand Down
8 changes: 4 additions & 4 deletions test/JacobianTest.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,19 +91,19 @@ end
for f in DiffBase.ARRAY_TO_ARRAY_FUNCS
v = f(X)
j = ForwardDiff.jacobian(f, X)
@test_approx_eq_eps j Calculus.jacobian(f, X, :forward) FINITEDIFF_ERROR
@test_approx_eq_eps j Calculus.jacobian(x -> vec(f(x)), X, :forward) FINITEDIFF_ERROR
for c in CHUNK_SIZES
cfg = JacobianConfig{c}(X)

println(" ...testing $f with chunk size = $c")
out = ForwardDiff.jacobian(f, X, cfg)
@test_approx_eq out j

out = similar(X, length(X), length(X))
out = similar(X, length(v), length(X))
ForwardDiff.jacobian!(out, f, X, cfg)
@test_approx_eq out j

out = DiffBase.JacobianResult(X)
out = DiffBase.DiffResult(similar(v, length(v)), similar(v, length(v), length(X)))
ForwardDiff.jacobian!(out, f, X, cfg)
@test_approx_eq DiffBase.value(out) v
@test_approx_eq DiffBase.jacobian(out) j
Expand All @@ -114,7 +114,7 @@ for f! in DiffBase.INPLACE_ARRAY_TO_ARRAY_FUNCS
v = zeros(Y)
f!(v, X)
j = ForwardDiff.jacobian(f!, zeros(Y), X)
@test_approx_eq_eps j Calculus.jacobian(x -> (y = zeros(Y); f!(y, x); y), X, :forward) FINITEDIFF_ERROR
@test_approx_eq_eps j Calculus.jacobian(x -> (y = zeros(Y); f!(y, x); vec(y)), X, :forward) FINITEDIFF_ERROR
for c in CHUNK_SIZES
cfg = JacobianConfig{c}(X)
ycfg = JacobianConfig{c}(zeros(Y), X)
Expand Down
27 changes: 20 additions & 7 deletions test/PartialsTest.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ samerng() = MersenneTwister(1)
for N in (0, 3), T in (Int, Float32, Float64)
println(" ...testing Partials{$N,$T}")

VALUES = ntuple(n -> rand(T), Val{N})
VALUES = (rand(T,N)...)
PARTIALS = Partials{N,T}(VALUES)

VALUES2 = ntuple(n -> rand(T), Val{N})
VALUES2 = (rand(T,N)...)
PARTIALS2 = Partials{N,T}(VALUES2)

##############################
Expand Down Expand Up @@ -70,7 +70,7 @@ for N in (0, 3), T in (Int, Float32, Float64)
@test hash(PARTIALS, hash(1)) == hash(copy(PARTIALS), hash(1))
@test hash(PARTIALS, hash(1)) == hash(copy(PARTIALS), hash(1))

const TMPIO = IOBuffer()
TMPIO = IOBuffer()
write(TMPIO, PARTIALS)
seekstart(TMPIO)
@test read(TMPIO, typeof(PARTIALS)) == PARTIALS
Expand All @@ -84,8 +84,8 @@ for N in (0, 3), T in (Int, Float32, Float64)
# Conversion/Promotion #
########################

const WIDE_T = widen(T)
const WIDE_PARTIALS = convert(Partials{N,WIDE_T}, PARTIALS)
WIDE_T = widen(T)
WIDE_PARTIALS = convert(Partials{N,WIDE_T}, PARTIALS)

@test typeof(WIDE_PARTIALS) == Partials{N,WIDE_T}
@test WIDE_PARTIALS == PARTIALS
Expand All @@ -101,8 +101,8 @@ for N in (0, 3), T in (Int, Float32, Float64)
@test (PARTIALS - PARTIALS).values == map(v -> v - v, VALUES)
@test getfield(-(PARTIALS), :values) == map(-, VALUES)

const X = rand()
const Y = rand()
X = rand()
Y = rand()

@test X * PARTIALS == PARTIALS * X
@test (X * PARTIALS).values == map(v -> X * v, VALUES)
Expand All @@ -111,6 +111,19 @@ for N in (0, 3), T in (Int, Float32, Float64)
if N > 0
@test ForwardDiff._mul_partials(PARTIALS, PARTIALS2, X, Y).values == map((a, b) -> (X * a) + (Y * b), VALUES, VALUES2)
@test ForwardDiff._div_partials(PARTIALS, PARTIALS2, X, Y) == ForwardDiff._mul_partials(PARTIALS, PARTIALS2, inv(Y), -X/(Y^2))

ZEROS = Partials((zeros(T, N)...))

@test (NaN * ZEROS).values == ZEROS.values
@test (Inf * ZEROS).values == ZEROS.values
@test (ZEROS / 0).values == ZEROS.values

@test ForwardDiff._mul_partials(ZEROS, ZEROS, X, NaN).values == ZEROS.values
@test ForwardDiff._mul_partials(ZEROS, ZEROS, NaN, X).values == ZEROS.values
@test ForwardDiff._mul_partials(ZEROS, ZEROS, X, Inf).values == ZEROS.values
@test ForwardDiff._mul_partials(ZEROS, ZEROS, Inf, X).values == ZEROS.values
@test ForwardDiff._mul_partials(ZEROS, ZEROS, Inf, NaN).values == ZEROS.values
@test ForwardDiff._mul_partials(ZEROS, ZEROS, NaN, Inf).values == ZEROS.values
end
end

Expand Down