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] mapmri with cvxpy 1.0.15 #1740

Merged
merged 2 commits into from Feb 15, 2019

Conversation

@skoudoro
Copy link
Member

commented Feb 14, 2019

This PR is a workaround to fix our problem with cvxpy 1.0.15 on Mapmri. More details on this issue cvxgrp/cvxpy#672.

I would like to know by the MapMRI expert (@rutgerfick @RafaelNH @arokem @Garyfallidis) if this approximation is ok or not.

Thank you for your feedback

@skoudoro skoudoro changed the title [FIX] mapmri [FIX] mapmri with cvxpy 1.0.15 Feb 14, 2019

@arokem

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Would we still need this if we skip this version of cvxpy and pin to 1.0.14 (as in #1695)?

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

I think so, even if we make Travis happy, any new users or if someone works with master and does an update of cvxpy, It will crash

@rutgerfick

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

Strange bug indeed.

Instead of arbitrarily setting the minimum lopt to 1e-20, perhaps it's cleaner to solve the problem without the regularization term if lopt == 0, and avoid the bug altogether.

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Ok, thank you for your feedback @rutgerfick. implemented on 014b57f

@rutgerfick

This comment has been minimized.

Copy link
Contributor

commented on 014b57f Feb 14, 2019

Perfect

@arokem

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

+1. Will merge as soon as the CI comes back

@arokem

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Thanks for implementing this!

@codecov-io

This comment has been minimized.

Copy link

commented Feb 14, 2019

Codecov Report

Merging #1740 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1740      +/-   ##
==========================================
+ Coverage   84.26%   84.26%   +<.01%     
==========================================
  Files         115      115              
  Lines       13606    13608       +2     
  Branches     2144     2145       +1     
==========================================
+ Hits        11465    11467       +2     
  Misses       1643     1643              
  Partials      498      498
Impacted Files Coverage Δ
dipy/reconst/mapmri.py 90.97% <100%> (+0.02%) ⬆️

@arokem arokem merged commit 04ddc4e into nipy:master Feb 15, 2019

4 checks passed

codecov/patch 100% of diff hit (target 84.26%)
Details
codecov/project 84.26% (+<.01%) compared to 43c8324
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the skoudoro:fix-cvxpy-1.0.15 branch Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.