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

fix #7 #8

Merged
merged 2 commits into from
Aug 2, 2014
Merged

fix #7 #8

merged 2 commits into from
Aug 2, 2014

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Jul 29, 2014

fixes #7

32 bit https://ci.appveyor.com/project/tkelman/ecos-jl/build/1.0.3/job/owy7fobin6qaslc3
64 bit https://ci.appveyor.com/project/tkelman/ecos-jl/build/1.0.3/job/vnfxkgon65el7mn9

I see quite a bit more test output on a Linux machine than I do on Win32, but the tests apparently aren't flagging any errors? And Win64 segfaults. Smells like an integer size problem, either in the Julia bindings or in ECOS itself.

@IainNZ
Copy link
Contributor

IainNZ commented Jul 29, 2014

@tkelman to the rescue :D

The verbosity of the solver is a compile-time thing so that might explain the reduced test output - the tests are pretty decent so if they pass, it works.

I was expecting you, if anything, to say that Win32 segfaults, because its only been used/tested on 64-bit linux and OSX so far!

@IainNZ
Copy link
Contributor

IainNZ commented Jul 29, 2014

It really is great that Win32 works though.
I wonder how to go about debugging the Win64 issues... Maybe going over the structs in src/types.jl with a fine-tooth comb...

@IainNZ
Copy link
Contributor

IainNZ commented Jul 29, 2014

One Win64 positive: the call to ECOS_ver at the start of test/direct.jl worked!

@tkelman
Copy link
Contributor Author

tkelman commented Jul 29, 2014

Kind of a drive-by, I saw Michael Grant posting on julia-users and was wondering what was up with CVX since the Stanford guys never followed up on my standing offer to do this.

I think the problem is more likely in upstream ECOS (see tons of warnings here https://gist.github.com/ced0bf8e2f0716472438, and the 64-bit ecostester.exe does not work, at least from MinGW GCC), with differences in the size of long, etc. AIUI ECOS considers using ANSI C instead of C99 a feature, so using actual portable fixed-size integers is harder. And they have Python bindings to worry about, the situation there with respect to integer size on Win64 is a complete and utter mess (http://mail.scipy.org/pipermail/numpy-discussion/2014-July/070726.html).

@IainNZ
Copy link
Contributor

IainNZ commented Jul 29, 2014

Yes we also saw that, haven't heard from CVX.jl people lately and not really sure what the development plan is. Assume he knows about what they've done and isn't doing an independent port. In the meantime, always good to have more solvers + have the conic interface ready for a CVX-style package to use.

@IainNZ IainNZ mentioned this pull request Jul 31, 2014
@echu
Copy link

echu commented Jul 31, 2014

Hey, sorry to hijack this thread a little bit: I saw mention of Michael Grant and CVX. I can probably surface some of your concerns / issues to him if you let me know what they are. As in, what do you @tkelman mean, "What's up with CVX?"

@tkelman
Copy link
Contributor Author

tkelman commented Jul 31, 2014

Maybe Iain's thoughts are different than mine, but in my case it was more like a "oh cool good to see him using Julia and asking questions." And I met Madeleine, Karanveer, and the rest of the CVX.jl group a few months ago at a Bay Area Julia Users meetup, and offered to help out with Windows binaries. But we haven't seen much activity since the end of the semester, which is maybe not so surprising.

@echu
Copy link

echu commented Jul 31, 2014

Oh, okay. :-p

Well, Madeleine is sharing an office with me (she's in SF today), but I'll
be sure to tell her that you guys want to know what's up with CVX.jl. :)

On Thu, Jul 31, 2014 at 10:32 AM, Tony Kelman notifications@github.com
wrote:

Maybe Iain's thoughts are different than mine, but in my case it was more
like a "oh cool good to see him using Julia and asking questions." And I
met Madeleine, Karanveer, and the rest of the CVX.jl group a few months ago
at a Bay Area Julia Users meetup, and offered to help out with Windows
binaries. But we haven't seen much activity since the end of the semester,
which is maybe not so surprising.


Reply to this email directly or view it on GitHub
#8 (comment).

@IainNZ
Copy link
Contributor

IainNZ commented Jul 31, 2014

@echu we (MIT JuliaOpt people) have met the CVX.jl team and we've been working with them to help them connect to the existing solvers. Re: Michael Grant and CVX, apart from sharing @tkelman s general happiness that he is trying Julia, just wanted to make sure he wasn't duplicating work on CVX.jl

@madeleineudell
Copy link

@tkelman: @karanveerm has been handling the windows integration issues; i'm not sure of the status, but it would be great to get your help!

@tkelman
Copy link
Contributor Author

tkelman commented Aug 1, 2014

I think the Julia wrappers are okay at this point. The C tests of ECOS itself have some trouble too on Win64, we're tracking that at embotech/ecos#68. Although it makes me wonder whether it has worked correctly with Windows 64-bit via Python or Matlab yet.

@IainNZ
Copy link
Contributor

IainNZ commented Aug 1, 2014

We could just merge this for now and get it out there for everyone not using Julia Win64 I suppose...

@jfsantos
Copy link
Contributor

jfsantos commented Aug 2, 2014

Since apparently the wrappers are ok and this is a problem in ECOS, I agree with @IainNZ that we could merge this. As soon as they get it fixed, hopefully we will just need to update the version string in our build script.

@tkelman
Copy link
Contributor Author

tkelman commented Aug 2, 2014

Okay by me. Someone (probably me? could also be cross-compiled and should come out the same) will have to build and upload new Windows binaries once the issue is fixed upstream, then we'll know for sure whether the Julia code here needs any changes for Win64.

@IainNZ
Copy link
Contributor

IainNZ commented Aug 2, 2014

OK, pulling trigger.

IainNZ added a commit that referenced this pull request Aug 2, 2014
@IainNZ IainNZ merged commit 4748a2f into jump-dev:master Aug 2, 2014
@tkelman tkelman deleted the appveyor branch August 2, 2014 21:15
IainNZ added a commit that referenced this pull request Aug 2, 2014
@tkelman
Copy link
Contributor Author

tkelman commented Aug 5, 2014

As I suspected it's an integer size problem. SuiteSparse_config.h uses C99 64-bit integers for indices on Win64, but ECOS has a lot of places (esp. in the tests) where it incorrectly uses long. Depending which way they decide to go, we may need to replace Clong with Int here in the Julia wrappers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Binaries for Windows
5 participants