cvxgrp / qcml

A Python parser for generating Python/C/Matlab solver interfaces
Other
42 stars 9 forks source link

Matlab/1 #25

Closed chinasaur closed 11 years ago

chinasaur commented 11 years ago

Hey, so here's a first pass for you to look at. It successfully stuffs params into data, but when I try to run ECOS on it I get Dual Infeasible. I notice that the stuff results from this look dualized compared to the direct ECOS feeding I had manually reverse engineered from CVX, so maybe something going on there...? Probably the thing to do is test the python and C versions with the real data now.

There was some ugliness around 0/1 indexing; see inline comments and commit logs. Basically I originally tried to just make everything 1 indexed, but there were Just objects that were used for indexing, so I didn't know if it would be okay to add 1 to those. Seemed like better not to, so I went back to 0 indexing in matlab_encoder and just add 1 to Gi Gj Ai Aj before calling sparse in functions_return to convert from triplet to column compressed. In matlab/codegen on the otherhand, it's all 1 indexed to start. I'm hoping this can be cleaned up but haven't figured it out yet.

Certainly possible that there are indexing issues due to the above, although the dimensions seem to come out right. Also likely this hack will not be compatible with future plans, if the 0 indexed stuff from matlab_encoder ever shows up somewhere other than Gi Gj Ai Aj, so we probably do need to resolve it. It seems to come down to Just, but there might be other things I missed.

I also tweaked cbp.py to take some helpful command line arguments. And I updated .gitignore.

chinasaur commented 11 years ago

Oh, and I left all the old stuff in MatlabCodegen but moved it to the end and docstringed or commented out things that actively conflicted. But presumably we can just clear all that old stuff?

echu commented 11 years ago

How are you calling ECOS, by the way?

echu commented 11 years ago

Okay; I just made several edits. I think the problem was that Matlab has

[i,j,v] = find(A)

while I was assuming you could do something like A.i, A.j, and A.v to get the information. I'm not sure if there's an elegant way around this.

If you look at the code (396b79b0c62ad6d9202ebc0f92f4892bb2c17a51), I have it computing the (0-based) indices by hand. Try it out now; the objective values agreed on the lasso problem I was trying.

And, yes, everything I commented out in the matlab/codegen.py file can now go away.

chinasaur commented 11 years ago

Ah, yes I was treating it as a dense matrix; silly of me. Not sure why that wouldn't work, but it's not good. I'll test. Thanks!

chinasaur commented 11 years ago

What do you think about adding to functions_setup some stuff to do the find and store it as params.name_i etc.?

echu commented 11 years ago

i think that works; give it a try.

On Thu, Sep 12, 2013 at 10:00 AM, Peter H. Li notifications@github.comwrote:

What do you think about adding to functions_setup some stuff to do the find and store it as params.name_i etc.?

— Reply to this email directly or view it on GitHubhttps://github.com/cvxgrp/qcml/pull/25#issuecomment-24337687 .

chinasaur commented 11 years ago

Now getting the right optval (although via problem dualized from my/CVX normal input)! I'm still not sure why treating the params as dense and stuffing a bunch of zeros broke things; probably there was some other indexing bug on top? Cleaning up some other bugs now and will add the find in functions_setup.

chinasaur commented 11 years ago

Do you have a take on the 0 indexing issue with Just? Seems like this could conflict with future plans, so would be nice to try to clear it up.

echu commented 11 years ago

Well, "Just" is used for both indices and values... so unfortunately, we have to stick with the current solution.

I'm actually planning on rewriting how coefficients are handled, so all this will change in the future (and what we've done now will inform the new design). I think this will make it easier to write the codegens.

If the Matlab code looks good, then it will become a permanent part of the repo! Thanks! Eric

On Thu, Sep 12, 2013 at 4:39 PM, Peter H. Li notifications@github.comwrote:

Do you have a take on the 0 indexing issue with Just? Seems like this could conflict with future plans, so would be nice to try to clear it up.

— Reply to this email directly or view it on GitHubhttps://github.com/cvxgrp/qcml/pull/25#issuecomment-24364018 .

chinasaur commented 11 years ago

Yeah, it looks like it basically works. Haven't checked socp2prob yet, but the optval looks right. Great!

I see I had failed to apply x.op in loop_over, which in CBP was a negation, so maybe that explains why the dense version was broken.

echu commented 11 years ago

the indexing was also off at some point, too. i ran the code and the indices were (-1,-1,-1,-1,...).

is the code any slower than the hand-written code you wrote in matlab?

On Thu, Sep 12, 2013 at 5:45 PM, Peter H. Li notifications@github.comwrote:

Yeah, it looks like it basically works. Haven't checked socp2prob yet, but the optval looks right. Great!

I see I had failed to apply x.op in loop_over, which in CBP was a negation, so maybe that explains why the dense version was broken.

— Reply to this email directly or view it on GitHubhttps://github.com/cvxgrp/qcml/pull/25#issuecomment-24366317 .

chinasaur commented 11 years ago

Hmm, I didn't see -1 indices. Well, it works now :).

The current code is about 4x slower; just the calls to sparse (i.e. cs_compress) take 2x as long as my old version. A bit surprising since my version just builds blocks and then concatenates them, similar to the SparseCompose C code I sent you.

Current code could be more efficient. We probably don't need all the repmat()s; is repeat only going to be used to repeat scalars? Without those it'll probably get down close to just the compress time.

echu commented 11 years ago

Yes; only scalars. The current Matlab code? You may not need all the calls to sparse--maybe only the last one that puts everything together.

Eric

On Thu, Sep 12, 2013 at 9:08 PM, Peter H. Li notifications@github.comwrote:

Hmm, I didn't see -1 indices. Well, it works now :).

The current code is about 4x slower, with just the calls to sparse (i.e. cs_compress) taking 2x as long as my old version. A bit surprising since my version just builds blocks and then concatenates them, similar to the SparseCompose C code I sent you. Current code could be more efficient. We probably don't need all the repmat()s; is repeat only going to be used to repeat scalars?

— Reply to this email directly or view it on GitHubhttps://github.com/cvxgrp/qcml/pull/25#issuecomment-24372005 .

chinasaur commented 11 years ago

It's just the two calls to sparse at the end. A is empty, but G by itself takes 2x longer than the hand coded version. So compress is more expensive than I thought, relative to concat. I guess makes sense because compress has to run through the whole input to count how many elements in each column and then do the copying, whereas concat just copies.

(The dimensions are slightly different for the two versions, but not enough to matter.)

I did throw a call to sparse into the assign function; not sure that's needed? It's not a time issue, but would like to cut it if it was superfluous.

echu commented 11 years ago

in matlab, it's superfluous. so i think you can just remove that.

huh. do you have the matlab profiling information? i'm surprised that one call to compress dominates the whole thing. i figured sparse matrix concatenation also needed to call compress / decompress as well....

On Thu, Sep 12, 2013 at 9:29 PM, Peter H. Li notifications@github.comwrote:

It's just the two calls to sparse at the end. A is empty, but G by itself takes 2x longer than the hand coded version. So compress is more expensive than I thought, relative to concat. I guess makes sense because compress has to run through the whole input to count how many elements in each column and then do the copying, whereas concat just copies.

(The dimensions are slightly different for the two versions, but not enough to matter.)

I did throw a call to sparse into the assign function; not sure that's needed? It's not a time issue, but would like to cut it if it was superfluous.

— Reply to this email directly or view it on GitHubhttps://github.com/cvxgrp/qcml/pull/25#issuecomment-24372500 .

chinasaur commented 11 years ago

https://gist.github.com/chinasaur/6546857

I originally thought Matlab might be doing a decompress/compress cycle as well, at least for vconcat. But after I wrote the hconcat/vconcat in SparseCompose I benchmarked it against Matlab's and they're very similar, so I'm pretty sure Matlab does it in a similar way.

chinasaur commented 11 years ago

Oh, and for Matlab's version the compress also has to sort by row within columns; CSparse should do better there.