search

jump-dev / ECOS.jl

A Julia interface to the ECOS conic optimization solver
https://github.com/embotech/ecos
Other
41 stars 17 forks source link

Support for mixed-integer/boolean problems (ECOS 1.1.0) #16

Closed adomahidi closed 1 year ago

adomahidi commented 9 years ago

We have pushed ECOS 1.1.0 yesterday, with the major change in the repository structure - the core ecos solver is now a submodule in all interfaces:

github.com/embotech/ecos (core C API)

Interface repositories:

github.com/embotech/ecos-matlab github.com/embotech/ecos-python

If there is any particular info you need from us to include support for binaries/integers please contact us (specifically, @hanwang who has developed the Branch-and-Bound module).

mlubin commented 9 years ago

Thanks for the update, it shouldn't be difficult at all to enable support for binary and integer variables. It's not typical for Julia packages to have git submodules, and it's not really necessary here since we distribute our own binaries on OS X and Windows.

adomahidi commented 9 years ago

Sure, whatever works best for you! Thanks!

tkelman commented 9 years ago

Note that the Win64 integer size fixes necessary to compile ecos properly were reverted. ecos needs to make up its mind and be consistent about integer sizes - pick one thing and do it everywhere including throughout the testing code, whether that's platform-dependent long or 64 bit integers everywhere. Or we'll have to build patched sources.

adomahidi commented 9 years ago

I think I have corrected the integer issue in Win64 in the current version (1.1.1). Can you please check and re-package that one? I see that the current ECOS.jl still has version number 1.0.5 if I'm not mistaken. Thanks!! Oh and if there are more issues with the ints please let me know.

mlubin commented 9 years ago

Thanks @adomahidi. Regarding the API changes for supporting integer variables, this is up for grabs by whoever needs it first :) CC @jfsantos @karanveerm @madeleineudell

tkelman commented 9 years ago

How many iterations is the lp_agg test expected to take? For 32-bit binaries built from Cygwin using CC='i686-w64-mingw32-gcc -g -Wl,--stack,8388608' AR=i686-w64-mingw32-ar make -e test shared I get this output from ecostester https://gist.github.com/909202d3864727746c9f

64 bit binaries built as CC='x86_64-w64-mingw32-gcc -g -Wl,--stack,8388608' AR=x86_64-w64-mingw32-ar make -e test shared result in a segfault https://gist.github.com/bbd13b7109cc7169f7fb

tkelman commented 9 years ago

ecos 2.0.1 will probably not segfault any more, but I disagree with the fix made in https://github.com/embotech/ecos/commit/8a26e941019e7808e9b3ad78695b8740fbc9383b - making your integer sizes depend on what compiler you're using is not a good idea, kind of the opposite of making up your mind. Pretty much means the C library's own tests aren't being run against visual studio, but that's not really our problem until a user wants to try an MSVC build of ecos along with the Julia bindings.

adomahidi commented 9 years ago

I'll try to explain the issue that we have in ECOS with integers, and maybe we can find a good solution together. The problem is that depending on which interface one is building, a sparse matrix comes in compressed column storage where the index arrays are integers that vary bitness on different platforms. So if we're compiling on Win64 for Matlab, then the arrays Matlab gives are 64bit, and we require to use "long ints". The issue is that in MSVC the long integer has 32bit, so you need to use "long long" (which is defined in the MSVC world as the _int64 type), whereas if you're using MinGW to compile then "long" has 64 bit size. This is at least what I have found out so far, and this is why ECOS makes the integer sizes depending on the build environment.

The cleanest solution would be to go for a predefined integer size (say int32), but then for example under Matlab Win64 we'd need to memcopy the data (a cast is not enough since sparseLDL and AMD operations perform pointer arithmetics). This would mean a performance loss, which in my view does not pay off the "cleanliness" of the solution of "making up our mind".

Of course, if you have a better proposal of how to deal with this in a better way, I'm all ears.

tkelman commented 9 years ago

whereas if you're using MinGW to compile then "long" has 64 bit size

This is incorrect.

tkelman commented 9 years ago

So the solution I think you should go for is to make the integer size configurable, with default choices that make sense for the target environment you're building for - whether that be Matlab, Python, or the standalone C library. What's missing is that in several of these configurations, most noticeably the standalone C library as we've been trying to build and report multiple times, certain integer sizes segfault because the code in ECOS is not consistent across the library and all tests. I get the distinct impression that few if any people build the library in "Matlab configuration," or "Python configuration," or even just the library by itself with MSVC, and run the C library's own tests. The environment that you're building the library for is not the same thing as the compiler that you're using to build the library. They're related in some cases, but not all.

adomahidi commented 9 years ago

The integer size IS configurable in the header file external/Suitesparse_config/Suitesparse_config.h. I just went through all tests and found 1 file that was using long instead of ECOS' integer type idxint. I will correct this; then all tests will be consistent with the right integer size.

As for testing, well, we run Python tests for the Python module, C tests for the C module, and Matlab tests for the Matlab module. Of course one could additionally run tests for the C core module every time any of the interfaces is built.

adomahidi commented 9 years ago

The test "norm.c" has been corrected in embotech/ecos@73f970c05b2e45c86707d3c28af03e80b94a33c8

tkelman commented 9 years ago

Patching a header file isn't exactly the best way of doing configuration, and you should still revert the compiler-dependent integer sizes, but yes it looks like 64-bit-ints with mingw will work now. The backtrace from February said exactly where the problem was, did it not?

adomahidi commented 9 years ago

Which post do you mean exactly? Apologies if this was slow, but if you know exactly where the problem is, why not create a PR next time?

tkelman commented 9 years ago

https://github.com/JuliaOpt/ECOS.jl/issues/16#issuecomment-72537761 - I was under the assumption you knew your source code better than I do. That pointed to the file in which the segfault was happening, and while I could've guessed it was an integer size problem, I didn't read the test files all that closely. I've also had a trivial 2-line PR open since March that hasn't been merged, so...

adomahidi commented 9 years ago

Yeah sorry for that one. I should have merged it right away.

tkelman commented 9 years ago

Anyway, now that that blocker is resolved, we can update the Windows and OSX binaries and the Julia bindings to use the latest version of ECOS pretty much at any time someone wants to work on that update. We can replace any instance of Clong in the Julia bindings with Int if we build the win64 binary using 64-bit integers. Should not be much work.

adomahidi commented 9 years ago

OK, thanks. Sorry again for not being responsive to this - try to do it better next time!

matbesancon commented 3 years ago

@mlubin should this issue be closed too then?

mlubin commented 3 years ago

Not sure. ecos_bb is still a missing feature, but not something I'd prioritize given my comment in https://github.com/jump-dev/ECOS.jl/pull/38#issuecomment-757542176.

odow commented 1 year ago

Closing because I don't see us adding this, and there has been no progress in 8 years.

If future reader has a need for this and it isn't met by using Pajarito.jl instead, please comment and I'll re-open.