AthenaEPI / dmipy

The open source toolbox for reproducible diffusion MRI-based microstructure estimation
MIT License
96 stars 30 forks source link

Merge pull request #1 from AthenaEPI/master #94

Closed Sara04 closed 4 years ago

Sara04 commented 4 years ago

Initial implementation of AmicoCvxpyOptimizer class. It contains sampling of parameter ranges and creation of parameter grids, creation of an observation matrix M (this will be moved to _create_forward_model_matrix in dmipy/core/modeling_framework.py) and the estimation of the vector containing distributions of the parameters

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 643


Files with Coverage Reduction New Missed Lines %
dmipy/utils/tests/test_spherical_convolution.py 1 95.35%
dmipy/signal_models/gaussian_models.py 3 95.2%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 632: -0.01%
Covered Lines: 4697
Relevant Lines: 5549

💛 - Coveralls
codecov-commenter commented 4 years ago

Codecov Report

Merging #94 into master will increase coverage by 0.05%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   81.45%   81.51%   +0.05%     
==========================================
  Files          64       64              
  Lines        5549     5549              
  Branches      658      658              
==========================================
+ Hits         4520     4523       +3     
+ Misses        852      848       -4     
- Partials      177      178       +1     
Impacted Files Coverage Δ
dmipy/distributions/tests/test_bingham.py 91.04% <0.00%> (ø)
dmipy/utils/tests/test_spherical_convolution.py 95.12% <0.00%> (+7.31%) :arrow_up:

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 c246fa7...5614e1b. Read the comment docs.

rutgerfick commented 4 years ago

Excellent. I'll take a look.

rutgerfick commented 4 years ago

one last thing is that you need to make the PEP8 line lengths and indentations pass. I don't know why it's not displaying it here but the lastest travis run on your merge request can be found here:

https://travis-ci.org/github/AthenaEPI/dmipy

You see that the following things are making the pipeline fail:

./dmipy/optimizers/amico_cvxpy.py:2:24: F821 undefined name 'optional_package'
./dmipy/optimizers/amico_cvxpy.py:100:80: E501 line too long (81 > 79 characters)
./dmipy/optimizers/amico_cvxpy.py:104:80: E501 line too long (82 > 79 characters)
./dmipy/optimizers/amico_cvxpy.py:107:80: E501 line too long (81 > 79 characters)
./dmipy/optimizers/amico_cvxpy.py:125:80: E501 line too long (82 > 79 characters)
./dmipy/optimizers/amico_cvxpy.py:131:60: F821 undefined name 'acquisition_scheme'
./dmipy/optimizers/amico_cvxpy.py:131:80: E501 line too long (87 > 79 characters)
./dmipy/optimizers/amico_cvxpy.py:134:21: E126 continuation line over-indented for hanging indent
./dmipy/optimizers/amico_cvxpy.py:136:21: E127 continuation line over-indented for visual indent

Just use pep8 to fix these small issues and for me we can merge it then :-)

rutgerfick commented 4 years ago

you still have 1 error:

./dmipy/optimizers/amico_cvxpy.py:2:24: F821 undefined name 'optional_package'

you need to import it first before you use it.