DNMY: Exposed variable new_x in C_wrapper. Maintains backward comp. - #313
DNMY: Exposed variable new_x in C_wrapper. Maintains backward comp.#313sohailreddy wants to merge 3 commits into
Conversation
…o Julia functions. Maintains backward compatibility for cases where new_x is not used by routines.
odow
left a comment
There was a problem hiding this comment.
What's the purpose of this?
We can't merge this as-is, because it's going to force all current models to throw an error on every function call, which will cause a massive performance problem.
Do you want to know if you need to recompute the function? You can do this in a closure.
function memoize_f(f)
last_x, last_fx = nothing, NaN
return (x) -> begin
if x == last_x
return last_fx
end
last_x = x
return f(x)
end
end
Codecov Report
@@ Coverage Diff @@
## master #313 +/- ##
==========================================
- Coverage 87.71% 87.07% -0.64%
==========================================
Files 3 3
Lines 692 712 +20
==========================================
+ Hits 607 620 +13
- Misses 85 92 +7
Continue to review full report at Codecov.
|
…ion, maintains backward compatibility and passes all test
|
Understood, I updated it to remove the exceptions. The update shouldn't affect performance much and it should be faster to use Ipopt's xnew rather than recomputing the norm/checking equivalence outside every time eval_* are called. |
|
What's the purpose of this? Do you have a benchmark where using a closure is a performance bottleneck? |
| prob.x .= x | ||
| end | ||
| new_obj = convert(Float64, prob.eval_f(x))::Float64 | ||
| new_x = convert(Bool, x_new) |
There was a problem hiding this comment.
In the spirit of keeping closer to the C interface, why not just pass x_new in as the Cint?
There was a problem hiding this comment.
On the Julia side, we might want to keep it closer to the Julia types?
There was a problem hiding this comment.
It's better to keep closer to the C types. Having differences between Julia and C is a pain (hence the reason you opened this PR).
| new_obj = convert(Float64, prob.eval_f(x))::Float64 | ||
| new_x = convert(Bool, x_new) | ||
| new_obj = nothing | ||
| if prob.expose_xnew |
There was a problem hiding this comment.
You can also write
new_obj = if prob.expose_xnew
convert(Float64, ...)
else
convert(Float64, ...)
endThat avoids the need for new_obj = nothing which creates a type instability.
There was a problem hiding this comment.
Then would this need to be done for the other routines? eval_f, eval_grad_f, eval_g, eval_jac_g, eval_h?
| m::Cint, | ||
| lambda_ptr::Ptr{Float64}, | ||
| ::Cint, | ||
| new_lambda::Cint, |
There was a problem hiding this comment.
| new_lambda::Cint, | |
| ::Cint, |
We don't name arguments if they aren't used. But if we're going th x_new route, should we not include new_lambda as well?
There was a problem hiding this comment.
Yes to be consistent, we should also expose new_lambda but I can't imagine many users using the exact hessian routines
|
I've updated this in #322, and added some basic tests. I'd still like to see a benchmark where this makes a difference. You'd have to cache the old solutions, so why not just cache and check the |
|
Closing in favor of #322. I'm still not convinced, but we can continue to discuss it in that PR. I'd like to see a practical application where this makes a difference. |
Updated C_wrapper to exposed the variable new_x from Ipopt routines to Julia functions. Maintains backward compatibility for cases where new_x is not used by routines.