Fix/segfaults #33
Fix/segfaults #33
Conversation
|
Thanks for your help on this! However, the solution for this is quite difficult as you already pointed out. I don't like that ipopt_adapter now depends on ipopt_solver. And there are other harder-coded definitions there. Can you definitely test that this is the source of the problem? I was under the impression that if "finite-difference-values" is set, the function below should never be called. ifopt/ifopt_ipopt/src/ipopt_adapter.cc Line 106 in b567a0e Then, the jacobian structure could only still come in here ifopt/ifopt_ipopt/src/ipopt_adapter.cc Line 42 in b567a0e but I also think that variable is not used internally by IPOPT if the "finite-difference-values" is set. Unfortunately i don't have a laptop with me, so can't test, can others confirm this problem? |
|
Hi Alex, I confirm that I get this issue as well. When providing empty implementations for the functions that fill Jacobians and using finite differences, I also get a segfault. The segfault happens in the ipopt, internally. Vivian will attach the gdb traceback. If the "finite-difference-values" option is set, the function is called once at the beginning to provide the structure of the jacobian to the ipopt. Since there is no structure provided, ipopt will subsequently segfault. Vivian will also update the pull request with a cleaner fix that doesn't make ipopt adapter dependent on the ipopt solver. |
|
Hi Alex, Following @jelavice's comment, here is the gdb traceback when running the original code with "finite-difference-values" option set and empty Jacobian implementation:
As you can see, Ipopt still expects the structure for the Jacobian and segfaults. I updated the code so ipopt_adapter no longer depends on ipopt_solver and simply gets a flag indicating whether the "finite-difference-values" option is set or not, which is a cleaner fix. |
There was a problem hiding this comment.
Hi @viviansuzano and @jelavice
Thanks for your help on this. I'd be great if you could still clarify the comments I left below and update the PR accordingly. Thanks! 👍
| * https://www.coin-or.org/Ipopt/documentation/node40.html | ||
| */ | ||
| void SetOption(const std::string& name, const std::string& value); | ||
| void SetOption(const std::string& name, int value); | ||
| void SetOption(const std::string& name, double value); | ||
|
|
||
| /** Get options of the IPOPT solver. |
There was a problem hiding this comment.
These two functions are not used if i'm not mistaken? so let's better remove them (and the .cc implementation) until we notice we need this call more often.
There was a problem hiding this comment.
You are right, they are not used anymore. I implemented them to read the solver options, but they turned out not to be necessary. I removed them from the .h and .cc files.
ifopt_ipopt/src/ipopt_adapter.cc
Outdated
| @@ -107,17 +112,29 @@ bool IpoptAdapter::eval_jac_g(Index n, const double* x, bool new_x, | |||
| Index m, Index nele_jac, Index* iRow, Index *jCol, | |||
| double* values) | |||
| { | |||
| // defines the positions of the nonzero elements of the jacobian | |||
There was a problem hiding this comment.
I think the above comment is more precise, let's keep it for now.
| ipopt_app_->Options()->GetStringValue("jacobian_approximation", value, ""); | ||
| if (value.compare("finite-difference-values") == 0) | ||
| finite_diff = true; | ||
|
|
There was a problem hiding this comment.
how about replacing line 76, 79, 80 with:
bool finite_diff = value=="finite-difference-values";
keeping declaration and definition closer together. Maybe also rename value to something like jac_type.
There was a problem hiding this comment.
Ok, good suggestion.
ifopt_ipopt/src/ipopt_solver.cc
Outdated
| @@ -98,4 +107,16 @@ IpoptSolver::SetOption (const std::string& name, double value) | |||
| ipopt_app_->Options()->SetNumericValue(name, value); | |||
| } | |||
|
|
|||
| void | |||
There was a problem hiding this comment.
All the lines below can be removed if I'm not missing something?
ifopt_ipopt/test/ex_test_ipopt.cc
Outdated
| @@ -44,7 +44,8 @@ int main() | |||
| // 2. choose solver and options | |||
| IpoptSolver ipopt; | |||
| ipopt.SetOption("linear_solver", "mumps"); | |||
| ipopt.SetOption("jacobian_approximation", "exact"); | |||
| //ipopt.SetOption("jacobian_approximation", "exact"); | |||
There was a problem hiding this comment.
For the test lets keep in exact, there the jacobians are defined.
|
Great, thanks a lot for your help @viviansuzano 👍 |
When the user chooses to use the "finite-difference-values" method for the "jacobian_approximation" option, it should be possible to leave the FillJacobianBlock function of the constraint class empty. However, Ifopt uses the Jacobian defined by the user to determine the number of non-zero elements in the constraint Jacobian, necessary information for interfacing the problem with Ipopt.
ifopt/ifopt_ipopt/src/ipopt_adapter.cc
Line 42 in b567a0e
The result is a Segmentation Fault when the constraint Jacobian was not defined by the user (because nnz_jac_g is defined as zero), even if "jacobian_approximation" is set to "finite-difference-values".
The solution I implemented is to use a dense Jacobian when the "finite-difference-values" option is set, which always works. The downside is, if the user did define the sparse structure for the Jacobian and the "finite-difference-values" option is set, Ifopt will still use a dense matrix instead of the sparse, which most likely will increase the computational cost.
Unless the number of non-zeros elements in the constraint Jacobian is also a user entry in Ifopt, I don't see a better way to fix this.