cvanaret / Uno

A next-gen Lagrange-Newton solver for nonconvex optimization. It unifies barrier and SQP methods in a modern and generic way, and implements different globalization flavors (line search/trust region and merit function/filter method/funnel method). Competitive against filterSQP, IPOPT, SNOPT, MINOS and CONOPT.
MIT License
297 stars 22 forks source link

Have Uno::solve() return an optional Result #109

Closed cvanaret closed 21 hours ago

cvanaret commented 5 days ago

cc @worc4021

Fixes https://github.com/cvanaret/Uno/issues/105.

worc4021 commented 5 days ago

This PR does address algorithmic access to the result. I'm happy with that part. My question would be: In the case where uno fails to find its initial point, is there anything that a user can do to figure out why?

imciner2 commented 5 days ago

Using a std::optional here would mean any future Uno library (whether shared or static) would require its consumers to be using C++17 or later also, since it is in the main API. I think that would also be a pain point for wrapping the API in Julia in the future should a shared library be made, since I don't think std::optional is supported in the wrapping tools yet (@amontoison do you know if it is?).

cvanaret commented 5 days ago

Using a std::optional here would mean any future Uno library (whether shared or static) would require its consumers to be using C++17 or later also, since it is in the main API. I think that would also be a pain point for wrapping the API in Julia in the future should a shared library be made, since I don't think std::optional is supported in the wrapping tools yet (@amontoison do you know if it is?).

Uno is clearly written in C++17. If I try to compile with C++11, I get a buuuunch of errors. Fortunately, we tested the build scripts with std::optional last week and as long as you don't use std::optional::value(), it seems to be fine :+1:

This PR does address algorithmic access to the result. I'm happy with that part. My question would be: In the case where uno fails to find its initial point, is there anything that a user can do to figure out why?

That's a good point. I'll think about a solution, looks like it would rule out std::optional<Result> anyway.

worc4021 commented 5 days ago

Maybe a suggestion that doesn't deviate too far from a std::optional<Result> in spirit:

Consider the following pattern for calling the solve:

uno::Uno uno = uno::Uno(*globalization_mechanism, options);
Result result{};
auto status = uno.solve(*model, initial_iterate, options, user_callbacks);
if (uno::AppStatus::Success == status) {
   result = uno.get_result();
} else {
   [...]
}

I reckon this way, most the work you've done can still be used and the event of a dramatic failure is then dealt with in the else case. Naturally, all the names are made up.. One thing that would be required for this is a that Result is default construable and movable/copyable, which I don't think it is atm.

amontoison commented 5 days ago

Using a std::optional here would mean any future Uno library (whether shared or static) would require its consumers to be using C++17 or later also, since it is in the main API. I think that would also be a pain point for wrapping the API in Julia in the future should a shared library be made, since I don't think std::optional is supported in the wrapping tools yet (@amontoison do you know if it is?).

I'm not sure if it's supported now, but we are on the edge in terms of support if it works.
I found a script on Yggdrasil where it was mandatory to install a new MacOS SDK:
https://github.com/JuliaPackaging/Yggdrasil/blob/cbaf27290b0a5ae5dc224ab820bec53359e2bdcc/M/mlpack/build_tarballs.jl#L21-L30

We may have updated to a more recent SDK recently, but I want to warn Charlie not to use very recent features of C++ because of compatibility issues with other libraries.
For example, Uno requires at least GCC 10, and compatibility only works in one direction:
Code compiled with an older version of GCC works on a more recent one, but the reverse is not true.

This is why we try to compile artifacts with GCC 4.8 on Yggdrasil — to maximize compatibility.

cvanaret commented 21 hours ago

Maybe a suggestion that doesn't deviate too far from a std::optional<Result> in spirit:

Thanks for the suggestion! I think this kind of design is confusing though (returning one value and having to query the other). I decided to:

cvanaret commented 21 hours ago

We may have updated to a more recent SDK recently, but I want to warn Charlie not to use very recent features of C++ because of compatibility issues with other libraries

Is C++17 very recent C++ though? 😄