Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

Conversation

cvanaret
Copy link
Owner

cc @worc4021

Fixes #105.

@worc4021
Copy link
Contributor

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

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

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 👍

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
Copy link
Contributor

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
Copy link
Contributor

amontoison commented Nov 20, 2024

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

cvanaret commented Nov 25, 2024

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

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? 😄

@cvanaret cvanaret closed this Nov 25, 2024
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.

Uno::solve() should return the result
4 participants