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

[ENH] Validate streamlines pre-LiFE #2195

Merged
merged 5 commits into from Jul 24, 2020
Merged

[ENH] Validate streamlines pre-LiFE #2195

merged 5 commits into from Jul 24, 2020

Conversation

dPys
Copy link
Contributor

@dPys dPys commented Jul 20, 2020

Maybe we could also add an optional brain mask argument to ensure we're only considering fibers inside the brain?

@dPys dPys mentioned this pull request Jul 20, 2020
@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

Merging #2195 into master will increase coverage by 2.21%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2195      +/-   ##
==========================================
+ Coverage   89.13%   91.35%   +2.21%     
==========================================
  Files         251      251              
  Lines       32427    32456      +29     
  Branches     3422     3412      -10     
==========================================
+ Hits        28905    29650     +745     
+ Misses       2806     2059     -747     
- Partials      716      747      +31     
Impacted Files Coverage Δ
dipy/tracking/life.py 94.85% <33.33%> (-3.40%) ⬇️
dipy/tracking/tests/test_life.py 98.11% <87.50%> (-1.89%) ⬇️
dipy/viz/regtools.py 31.49% <0.00%> (ø)
dipy/reconst/tests/test_shm.py 99.41% <0.00%> (+0.12%) ⬆️
dipy/io/utils.py 86.41% <0.00%> (+0.54%) ⬆️
dipy/workflows/stats.py 85.81% <0.00%> (+0.70%) ⬆️
dipy/nn/model.py 92.59% <0.00%> (+1.85%) ⬆️
dipy/nn/tests/test_tf.py 88.46% <0.00%> (+1.92%) ⬆️
dipy/reconst/shm.py 92.59% <0.00%> (+3.33%) ⬆️
... and 11 more

Copy link
Contributor

@arokem arokem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I have a suggestion to make this a little less auto-magical. Also, would you mind adding a test?

dipy/tracking/life.py Outdated Show resolved Hide resolved
Co-authored-by: Ariel Rokem <arokem@gmail.com>
@pep8speaks
Copy link

pep8speaks commented Jul 20, 2020

Hello @dPys, Thank you for updating !

Line 473:13: E128 continuation line under-indented for visual indent

Line 104:19: E128 continuation line under-indented for visual indent
Line 105:33: E128 continuation line under-indented for visual indent
Line 114:13: E128 continuation line under-indented for visual indent

Comment last updated at 2020-07-23 22:57:02 UTC

@dPys
Copy link
Contributor Author

dPys commented Jul 20, 2020

Test added

@arokem
Copy link
Contributor

arokem commented Jul 20, 2020

Looks like there's some problem with the fixture you added? https://travis-ci.com/github/dipy/dipy/jobs/362957473#L2288

@dPys
Copy link
Contributor Author

dPys commented Jul 20, 2020

Hmm that's odd. I'm trying to test this module, but when I run pytest -vvv test_life.py, I get:

Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/local/anaconda3/lib/python3.7/site-packages/_pytest/python.py:511: in _importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=importmode)
/usr/local/anaconda3/lib/python3.7/site-packages/py/_path/local.py:701: in pyimport
    __import__(modname)
/usr/local/anaconda3/lib/python3.7/site-packages/_pytest/assertion/rewrite.py:152: in exec_module
    exec(co, module.__dict__)
test_life.py:3: in <module>
    import dipy.core.gradients as grad
../../core/gradients.py:10: in <module>
    from dipy.core.sphere import disperse_charges, HemiSphere
../../core/sphere.py:9: in <module>
    from dipy.reconst.recspeed import remove_similar_vertices
E   ModuleNotFoundError: No module named 'dipy.reconst.recspeed'

Have you encountered this error before? It's strange because no relative imports are used in the file and I can easily import dipy.reconst.recspeed in ipython so the install is good...

@skoudoro skoudoro added this to the 1.2 milestone Jul 21, 2020
@arokem
Copy link
Contributor

arokem commented Jul 23, 2020

Yo @dPys: are you sure you are running this in the same env as the import in python? This looks like what I get when I don't have the cython extensions compiled. Could you please try compiling them again (e.g., python setup.py build_ext --inplace)?

@skoudoro
Copy link
Member

This looks like what I get when I don't have the cython extensions compiled

+1, I encounter this when there is an environment problem

Can you rebase your PR? Furthermore, I recommend to keep the test simple so to avoid the use of pytest.parametrize. Can you update your test without this decorator ?
Thank you

@dPys
Copy link
Contributor Author

dPys commented Jul 23, 2020

Thanks @arokem and @skoudoro , that was totally what it was!

Compiling, rebasing, and will update test per everyone's suggestions... Stay tuned

@dPys
Copy link
Contributor Author

dPys commented Jul 23, 2020

Done

@arokem
Copy link
Contributor

arokem commented Jul 23, 2020

LGTM. I think we can merge when the CI comes back green.

@skoudoro
Copy link
Member

CI's are green, Thank you @dPys! merging

@skoudoro skoudoro merged commit 2a44ea0 into dipy:master Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants