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

WIP: replacing cvxopt with cvxpy. #1180

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@arokem
Member

arokem commented Feb 20, 2017

Following up on #1167

@arokem

This comment has been minimized.

@coveralls

This comment has been minimized.

coveralls commented Feb 20, 2017

Coverage Status

Coverage decreased (-0.4%) to 87.904% when pulling daaf5c0 on arokem:cvxpy into 5da8fa5 on nipy:master.

@grlee77

This comment has been minimized.

Contributor

grlee77 commented Feb 20, 2017

I assume the motivation for switching to CVXPY is for the Apache license terms (as opposed to the GPL-licensed CVXOPT)? In that case, does it really help if the underlying GPL-licensed ECOS solver is still being used by CVXPY here?

It looks like CVXPY can also use the MIT-licensed SCS solver instead of ECOS, but I am not familiar enough with the solvers to know whether it is a suitable substitute for ECOS in this case.

@codecov-io

This comment has been minimized.

codecov-io commented Feb 21, 2017

Codecov Report

Merging #1180 into master will decrease coverage by 0.52%.
The diff coverage is 12.5%.

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
- Coverage   85.93%   85.41%   -0.53%     
==========================================
  Files         219      217       -2     
  Lines       26414    26039     -375     
  Branches     2714     2670      -44     
==========================================
- Hits        22699    22240     -459     
- Misses       3053     3147      +94     
+ Partials      662      652      -10
Impacted Files Coverage Δ
dipy/reconst/tests/test_mapmri.py 89.73% <50%> (-8.84%) ⬇️
dipy/reconst/mapmri.py 81.45% <7.14%> (-9.03%) ⬇️
dipy/viz/window.py 58.84% <0%> (-2.31%) ⬇️
dipy/viz/interactor.py 96.1% <0%> (-1.3%) ⬇️
dipy/data/__init__.py 89.33% <0%> (-0.21%) ⬇️
dipy/direction/tests/test_peaks.py 99.42% <0%> (-0.03%) ⬇️
dipy/segment/tests/test_clustering.py 99.38% <0%> (-0.01%) ⬇️
dipy/workflows/tests/test_masking.py 91.66% <0%> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd4a17b...299c13d. Read the comment docs.

@arokem

This comment has been minimized.

Member

arokem commented Feb 21, 2017

Jokes on me, huh? Well, FWIW, SCS is inaccurate in an identical way to ECOS.

[edited to add:] So let's start working from that!

@coveralls

This comment has been minimized.

coveralls commented Feb 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 87.904% when pulling daaf5c0 on arokem:cvxpy into 5da8fa5 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Feb 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 87.904% when pulling dc28765 on arokem:cvxpy into 5da8fa5 on nipy:master.

@arokem arokem referenced this pull request Mar 6, 2017

Open

Nf mtms csd model #1168

@arokem

This comment has been minimized.

Member

arokem commented Mar 6, 2017

Hey @matthew-brett : what do we need to do in order to get cvxpy into the wheel-house? Thanks!

@arokem arokem force-pushed the arokem:cvxpy branch from fe4145f to c2f70e6 Mar 23, 2017

@arokem

This comment has been minimized.

Member

arokem commented Mar 23, 2017

@arokem arokem force-pushed the arokem:cvxpy branch from 2e8b61c to ed6c0e6 Mar 23, 2017

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Apr 9, 2017

While people are on the subject of switching solvers, please make sure that the replacement would at least be as accurate as cvxopt, these thing are always super hard to debug when you have a midway loss of precision due to who knows what.

Also, if you really, really just want to avoid it, scipy.optimize.nnls, but the speed is abysmal (like, unusable in practice when called a million times) because it is an all around purpose solver. But other implementation of the same algorithm might exist elsewhere (although a well done convex formulation will always beat it, this one does the old fashion way we learned of solving the KKT conditions for problems on paper.)

Does not look like sklearn ridge solvers does constraints or positivity though, but they might have some stuff in that area.

@arokem

This comment has been minimized.

Member

arokem commented Apr 9, 2017

Thanks for your input. Help figuring out specifically how to make ECOS work here would be very welcome.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Apr 9, 2017

In what sense? I thought it was amongst the few solvers that came out of the box with cvxpy, unless they changed stuff when they changed the license also (it used to be gpl not too long ago, well I checked it a few years back already though).

@arokem

This comment has been minimized.

Member

arokem commented Apr 9, 2017

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Apr 10, 2017

Not sure what you mean, cvxpy is designed as a easy to use frontend to many solvers. You write stuff in its syntax, and it does the job of using the best available option for you, as not all solvers are equally good at everything.

And as you do here you force it to use scs in particular, but it seems to not be able to solve the problem in some cases (which is expected somewhat, because if not there would be only the need for a single solver to do everything, the cool part about cvxpy is that you don't have to bother with those details).

Anyway, that's what I got from your problem. If not, well it builds (unlike ecos, were the instructions on windows is basically give up https://www.embotech.com/ECOS/Install).

Does it work if you do not force it to use a solver? Might want to check what it does use under the hood and if it's the same in all cases. I also have stuff to prepare for ismrm, so I won't likely be able to help out for stuff if it was a pressing matter though.

@arokem

This comment has been minimized.

Member

arokem commented Apr 10, 2017

@skoudoro

This comment has been minimized.

Member

skoudoro commented Aug 9, 2017

I think, it is done via #1285, we close this PR?

@arokem

This comment has been minimized.

Member

arokem commented Aug 10, 2017

yes. this was superseded by #1285

@arokem arokem closed this Aug 10, 2017

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