Skip to content

expand NaN-safe mode to cover individual zeros in Partials#386

Closed
jrevels wants to merge 3 commits into
masterfrom
jr/nanstuff
Closed

expand NaN-safe mode to cover individual zeros in Partials#386
jrevels wants to merge 3 commits into
masterfrom
jr/nanstuff

Conversation

@jrevels

@jrevels jrevels commented Feb 12, 2019

Copy link
Copy Markdown
Member

This likely further decreases performance when NaN-safe mode is engaged, but I think it's more correct so it's worth the perf hit either way. NaN-safe mode is already the expected slow path so this shouldn't change user expectations too much.

I'll do at least a simple benchmark to get a ballpark performance impact for the docs. Also need to add a test or two.

cc @andreasnoack this fixes your example; does it help with whatever actual problem you were encountering?

@jrevels

jrevels commented Feb 19, 2019

Copy link
Copy Markdown
Member Author

Interestingly, it looks like this actually improves the performance ratio between NaN-safe mode and non-NaN-safe mode for a simple rosenbrock gradient check (code below). I'm not sure why though, and the current performance quote of 5%-10% is definitely no longer correct; on master, right now, the performance hit is closer to 5x (ouch). It's about 3x with this PR.

At this point, I'm not confident enough to provide a specific estimate of the performance impact in the docs, so I'm going to make the current statement there more vague.

using ForwardDiff, BenchmarkTools

function rosenbrock(x::Vector)
    a = 1.0
    b = 100.0
    result = 0.0
    for i in 1:length(x)-1
        result += (a - x[i])^2 + b*(x[i+1] - x[i]^2)^2
    end
    return result
end

x = rand(1000)
@benchmark ForwardDiff.gradient(rosenbrock, $x)

@devmotion devmotion mentioned this pull request Oct 1, 2025
@DilumAluthge DilumAluthge deleted the jr/nanstuff branch April 22, 2026 16:54
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.

1 participant