jump-dev / KNITRO.jl

A Julia interface to the Artelys Knitro solver
https://www.artelys.com/knitro
Other
78 stars 23 forks source link

Rewrite the C wrapper #268

Closed odow closed 10 months ago

odow commented 10 months ago

Part of #267 Currently failing because things are still only half-fixed, but it's enough to get a sense for discussion.

I've learnt a lot about the code. The main issues:

It all looks very familiar, because it is exactly how a lot of the early Cbc/Clp/GLPK/Gurobi/CPLEX wrappers were written. In the long run though, I think it was a bad choice. The MILP wrappers are a lot more maintainable now that we switched to libxxx.jl + MOI. And you can still call any C function, it just takes a little more overhead, but it's also much clearer exactly what you are doing and why.

codecov[bot] commented 10 months ago

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (4db649c) 74.57% compared to head (1be5603) 88.61%.

Files Patch % Lines
src/MOI_wrapper.jl 87.59% 17 Missing :warning:
src/C_wrapper.jl 97.90% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #268 +/- ## =========================================== + Coverage 74.57% 88.61% +14.04% =========================================== Files 5 3 -2 Lines 1805 1019 -786 =========================================== - Hits 1346 903 -443 + Misses 459 116 -343 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

odow commented 10 months ago

I've asked the Artelys folks if they have people using the current low-level API. That would be one blocker moving forward with this. (We could keep the old functions if needed. But only advertise the "proper" way of using the API going forward.)

mgabay commented 10 months ago

Hi Oscar,

We can't think of any client who could be using it currently.

Anyway, I've been looking into the discussion in #267 and #268 and the proposal makes a lot of sense! I think it is a very good idea to go on with these changes, especially as 1.0 has not been released yet.

Also, I expect it would make it easier to reflect possible additions to Knitro API in the future. Don't worry, no breaking changes coming in foreseeable future on our side ;-)

Thanks a lot for your work on this!

odow commented 10 months ago

Also, I expect it would make it easier to reflect possible additions to Knitro API in the future

Yes, precisely. This change would make it so there is no friction between changes/updates/new features in the C API and Julia. They would be exactly equivalent.

This is the approach we have taken with Gurobi.jl etc, and it works very nicely.

odow commented 10 months ago

Since this is getting rather big, I'm going to merge this for now. I'll make a separate PR that fixes all of the return code checking.