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

cvxportfolio upgrade for cvxpy version 1.0.3 #11

Closed
sunilshah opened this issue May 31, 2018 · 15 comments
Closed

cvxportfolio upgrade for cvxpy version 1.0.3 #11

sunilshah opened this issue May 31, 2018 · 15 comments

Comments

@sunilshah
Copy link

cvxportfolio code needs to be upgraded for cvxpy 1.0.3 (current default in PyPi). Current examples and notebook fail as expected. Any plans to upgrade and test?

@mladendalto
Copy link

I second @sunilshah 's request.
For me HalloWorld.ipynb crashes at
sum_entries and mul_elemwise,
which do not exist in new cvxpy.

@ghost
Copy link

ghost commented Jun 20, 2018

Yes, thanks for noting this. I'll move to 1.0 asap.

@wec7
Copy link
Contributor

wec7 commented Jun 27, 2018

This can be closed.

@sunilshah
Copy link
Author

sunilshah commented Jun 27, 2018

Thanks!

The upgraded version fails on two items in my tests.

  1. MultiPeriodOptimization.ipynb

fails at line 353 in policies.py

      353  z = cvx.Variable(*w.size)
      354  wplus = w + z

changing it to

            z=cvx.Variable(*w.shape)

fixes it on my system (homebrew, mac os, python3, all latest versions). That makes sense, given the changes in cvxpy.

  1. All tests fail when cvxportfolio is installed using

        pip3 install -U cvxporfolio
        nosetests cvxporfolio
    

I am using homebrew package manager to install python3.

I clone the repository and run the cloned version of the software rather than the one installed using pip3. The cloned version runs nosetests fine in the test directory.

@wec7
Copy link
Contributor

wec7 commented Jun 29, 2018

Perhaps submit a pull request to @enzobusseti regarding item 1?

Regarding item 2, I think tests on the pip installed lib is expected due to missing data (I may misunderstand). It was failing even before.

@wec7
Copy link
Contributor

wec7 commented Jun 29, 2018

@enzobusseti thanks for taking my pull request.

But there might be some potential problems with this upgrade (not captured by the tests), I use your lib for portfolio optimization in actual investment, and recently realized the solver outputs differently (single period optimization policy). From my result speaking, older version works correctly while upgraded version must have something wrong.

I haven't dig into details yet, it might be caused by cvxpy rather than cvxportfolio, just a note here first.

@sunilshah
Copy link
Author

@weiyialanchen

New definition of leverage could be a reason for the difference. cvxpy 1.0.x uses OSQP as the default QP solver while cvxpy 0.4.11 used ECOS. In my tests comparing OSQP with ECOS, I have found OSQP to be more robust, though when both solvers worked, the results were identical within tolerances.

@ghost
Copy link

ghost commented Jul 2, 2018

@sunilshah regarding the test failing, I found an error in the pip package (I wasn't including the test data), which is fixed now. The error only affects the test, not the rest of the library.

@weiyialanchen I changed the definition of leverage (there was a subtle mismatch with the definition in the paper). It shouldn't have big effects, but you might get slightly different numerical results if your strategy uses the leverage limit.

Solvers: OSQP is a new but very good solver, and should have noticeably faster convergence than ECOS. I'm planning to make the default TCostModel quadratic (instead of 3/2 power) so that it compiles to a quadratic program and is solved by OSQP.

@sunilshah
Copy link
Author

@enzobusseti

Thanks for fixing tests.

As for changing to OSQP as a default, please wait till issue #535 in cvxpy for OSQP is resolved (or you can use the workaround of changing maximization to minimization).

@wec7
Copy link
Contributor

wec7 commented Jul 2, 2018

@enzobusseti @sunilshah

I didn't use a leverage limit. But after changing to ECOS I got my expected output back, thank you guys!!

@optwave
Copy link

optwave commented Aug 4, 2018

@weiyialanchen

and recently realized the solver outputs differently (single period optimization policy)

It seems that the outputs of the first experiment with the coarse grid are different from the paper for both SinglePeriodOpt (worse in some cases) and MultiPeriodOpt (better in some cases). AFAICS, I use the latest versions of cxvportfolio and cvxpy. Please find below the corresponding figures.

SinglePeriodOpt for "2012-01-01"-"2016-12-31" (note that the scaling for y-axis is different from the one given in the paper where max(y-axis) was 30%):
spo_riskrewardfrontier_5y

MultiPeriodOpt for "2012-01-01"-"2016-12-31":
mpo2_riskrewardfrontier_5y

BTW, in MultiPeriodOptimization.ipynb I actually use
353 z = cvx.Variable(w.size)
and not
353 z = cvx.Variables(*w.shape)
as suggested by @sunilshah. The latter caused
AttributeError: module 'cvxpy' has no attribute 'Variables'

I am not sure what the differences are due. It would make sense to benchmark the results under both versions of cvxpy and different versions of cvxportfolio and preferably by a person who understands the internals (I am not the right person in this sense).

Assuming that the results that I get from MultiPeriodOpt are correct, it appears that the revenue results for 10% risk are now ~35% and not ~23% as in Figure 7.5 of the paper, it would be nice to fully understand the origins of this non-trivial difference.

@bstellato
Copy link
Member

@sunilshah looks like the cvxpy issue is now resolved https://github.com/cvxgrp/cvxpy/issues/535

@sunilshah
Copy link
Author

sunilshah commented Aug 13, 2018

@bstellato

Yes, it appears to be fixed.

@optwave

Thanks for pointing out my typo, which is fixed in my comment now.

@enzobusseti

It may be worth testing out OSQP as the default solver now.

@ghost
Copy link

ghost commented Aug 14, 2018

@optwave , thanks for reporting. it might also be related to #14 (there was a bug in the definition of leverage limit, it makes sense if the results changed a bit).

If there is a $3/2$ transaction cost model the problem should compile to SOCP and default to ECOS. To force OSQP you can pass solver = 'OSQP' to the SPO policy object (but that will raise an exception if you use the $3/2$ tcost). (I believe OSQP is preferable, but it's up to the user.)

cvx.Variables is a typo, feel free to make a pull request..

@quant5
Copy link
Contributor

quant5 commented May 24, 2023

@enzbus can this old issue be closed? https://github.com/cvxgrp/cvxportfolio/blob/master/setup.py#L21

@enzbus enzbus closed this as completed Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants