Skip to content

Conversation

@jrevels
Copy link
Member

@jrevels jrevels commented Feb 12, 2019

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
Copy link
Member Author

jrevels commented Feb 19, 2019

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
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.

2 participants