-
Notifications
You must be signed in to change notification settings - Fork 295
Description
There is some kind of (longstanding, see below) typo or copy&paste error in Jipopt::finalize_solution
that leads to the objective function value not being copied correctly: the code (at src/Interfaces/IpStdJInterface.cpp:668
) reads:
env->GetNumberArrayRegion(fj, 0, 1, &obj_value);
where it should read:
env->SetNumberArrayRegion(fj, 0, 1, &obj_value);
(note Set, not Get).
This has been broken since basically forever. At DAGOPT, we had carried a local workaround for that in our wrapper classes since 2017 (when we first noticed the issue, but had not yet found the bug in the code), and a local patch for Ipopt 3.12.13 since 2019 (when I realized the fix was literally one character). Unfortunately, it looks like I had somehow forgotten to report this upstream, and it looks like nobody else noticed it either, because it is still broken in the current 3.14.17 release and the current stable/3.14 branch. So I am reporting it now. Better than never, I guess.
Note that the code in 3.12.x was using GetDoubleArrayRegion
, this was since updated to use the GetNumberArrayRegion
macro, but the wrong "Get" that should be "Set" was unfortunately not noticed when that change was made either.
GetNumberArrayRegion
(i.e., GetDoubleArrayRegion
) cannot possibly be the right method to use here, because it copies the current value of fj
to the by-value argument obj_value
of Jipopt::finalize_solution
that is discarded immediately thereafter when the method returns. Changing it to SetNumberArrayRegion
should do the right thing (copying from the obj_value
argument to fj
, as for all the other arguments) and be an obvious typo fix. (I can definitely confirm it fixed the issue in 3.12.13. I have not yet tested it in the current 3.14.17 because I am working right now on porting our build scripts, which is how I have noticed that the bug is still present.)