WIP: remove unused and untested code

[odow]

I'm trying to make sense of the C code, but I am hitting a wall of unused and untested methods with a massive variety of input arguments. I don't think we should merge this (yet), but I'm going to have a go at pruning everything. If we merge some variation of this, it could be v2.0.0.

[codecov]

Codecov Report

Base: 56.96% // Head: 93.26% // Increases project coverage by +36.30% 🎉

Coverage data is based on head (388022b) compared to base (64266b6).
Patch coverage: 95.85% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           master      #86       +/-   ##
===========================================
+ Coverage   56.96%   93.26%   +36.30%     
===========================================
  Files           9        4        -5     
  Lines         811      416      -395     
===========================================
- Hits          462      388       -74     
+ Misses        349       28      -321     

Impacted Files	Coverage Δ
src/c_api.jl	93.63% <93.63%> (ø)
src/CSDP.jl	100.00% <100.00%> (ø)
src/MOI_wrapper.jl	92.44% <100.00%> (+2.02%)	⬆️
src/c_api_patch.jl	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[odow]

Okaaaaay. Now I understand things. We had +2,300 lines of stuff floating around not actually doing anything.

There are also two separate APIs.

1. The API for MOI
2. The API for easy_sdp called here: https://github.com/jump-dev/CSDP.jl/blob/master/examples/example.jl

Here's the calls used by MOI: https://github.com/jump-dev/CSDP.jl/blob/od/rm-code/src/c_api.jl
Here's the calls used by easy_sdp: https://github.com/jump-dev/CSDP.jl/blob/od/rm-code/src/simple_api.jl

[odow]

Making things worse, for MOI we're not actually calling CSDP, but this patch of @blegat:
https://github.com/JuliaPackaging/Yggdrasil/blob/master/C/Coin-OR/CSDP/bundled/patches/blegat.patch

so that could be the cause of the issue.

[blegat]

Would it be possible to keep this feature ?

[odow]

What is the motivation? Where/when is it used?

[blegat]

When I am solving an SDP and why to understand better the behavior of the solver, it is useful to get a peak at the internal primal and dual solutions.

[blegat]

Making things worse, for MOI we're not actually calling CSDP, but this patch of @blegat: https://github.com/JuliaPackaging/Yggdrasil/blob/master/C/Coin-OR/CSDP/bundled/patches/blegat.patch

so that could be the cause of the issue.

Yes, that patch provides an API for CSDP that is more natural to call from Julia and also fixed the segfault we had in #39

[odow]

Yes, that patch provides an API for CSDP that is more natural to call from Julia and also fixed the segfault we had in #39

My tentative conclusion is that a better fix is to switch to a one-shot optimize!(csdp, src) and use the easy_sdp interface. That should let us build all of the vectors in one go without worrying about the GC issues. And the we should drop the patch from CSDP_jll.

Having these two APIs, especially because the current MOI one is undocumented, is confusing, to say the least.

It took me longer than I'd like to admit to find the patch in CSDP_jll. I was like, wtf, how come loaded_initsoln is not in coin-or/Csdp. I could find initsoln, so then I went looking for some C macro magic that prefixed loaded_ with some different arguments somewhere...

[blegat]

My tentative conclusion is that a better fix is to switch to a one-shot optimize!(csdp, src) and use the easy_sdp interface. That should let us build all of the vectors in one go without worrying about the GC issues. And the we should drop the patch from CSDP_jll.

What would that achieve. The current solution works, copies directly the MOI data into the CSDP internal datastructure by passing scalar values so it's quite simple and easy to be segfault-free and reason about in terms of pointers, references and so on.
The issue with easy_sdp IIRC was that you had to create the matrices in the Julia side in a compatible way. That was messy and easily lead to segfault including #39 for which we could never find the issue.
I agree we could try to simplify and replace the C wrapper by an automatically generated one with Clang and remove the helpers for using the C interface directly from Julia that are not automatically generated. Except for the block matrix interface which is quite useful to read them directly as if they were Julia matrices. I even thought at some point to integrate it with JuliaArrays/BlockDiagonals.jl#20

However, I think it's best to keep using that interface, I designed it specifically to make the MOI wrapper simple and efficient and clean. Indeed, we could document better that we are using a patch but I don't think using easy_sdp would simplify things, it would make the wrapper much more complicated.

[odow]

What would that achieve. The current solution works, copies directly the MOI data into the CSDP internal datastructure by passing scalar values so it's quite simple and easy to be segfault-free and reason about in terms of pointers, references and so on.

I think there's a problem somewhere in it: #74 (comment)

[blegat]

Might be but then from experience, working with the patch will be easier than with the easy_sdp. easy_sdp is easy to use from C but not from Julia. Don't throw the baby out with the bathwater ^^

[odow]

Closing as won't implement.