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

Corrected median_otsu function declaration that was breaking tutorials #1836

Merged
merged 4 commits into from Jun 12, 2019

Conversation

@clintg6
Copy link
Contributor

commented May 16, 2019

The prior function declaration breaks all these tutorials that use median_otsu:
brain_extraction_dwi.py
reconst_msdki.py
reconst_dti.py
reconst_csa_parallel.py
reconst_fwdti.py
reconst_csa.py
reconst_csd_parallel.py
reconst_dki.py
syn_registration_2d.py
syn_registration_3d.py
tracking_quick_start.py

You can test this by running any of these scripts in dipy/doc/examples or here http://nipy.org/dipy/examples_built/reconst_csa.html. My modification corrects this so that all the tutorials still function normally.

@skoudoro

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Good catch! Thank you for this!

LGTM. I see that @arokem introduce this change here (PR #1821) so is that ok for you or you prefer to keep your previous change and update all tutorials?

@codecov-io

This comment has been minimized.

Copy link

commented May 17, 2019

Codecov Report

Merging #1836 into master will decrease coverage by 0.69%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1836     +/-   ##
=========================================
- Coverage   84.28%   83.58%   -0.7%     
=========================================
  Files         117      117             
  Lines       14222    14215      -7     
  Branches     2245     2244      -1     
=========================================
- Hits        11987    11882    -105     
- Misses       1718     1812     +94     
- Partials      517      521      +4
Impacted Files Coverage Δ
dipy/stats/analysis.py 38.19% <0%> (-36.81%) ⬇️
dipy/workflows/stats.py 61.6% <0%> (-30.4%) ⬇️
dipy/viz/__init__.py 80% <0%> (-10%) ⬇️
dipy/viz/regtools.py 32% <0%> (-4.8%) ⬇️
dipy/utils/optpkg.py 69.56% <0%> (-4.35%) ⬇️
dipy/io/vtk.py 12.5% <0%> (-2.09%) ⬇️
dipy/viz/panel.py 81.69% <0%> (-1.41%) ⬇️
dipy/viz/app.py 51.96% <0%> (-0.4%) ⬇️
dipy/data/fetcher.py 32.88% <0%> (-0.19%) ⬇️
@clintg6

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

@skoudoro I looked over the PR 1821 and ran some further tests. The modification to the older declaration passes the following workflow and segment tests.

  1. dipy/workflows/tests/test_segment.py
  2. dipy/workflows/tests/test_workflow.py
  3. dipy/segment/tests/test_mask.py

I also looked further into dipy/workflows/segment.py. The ordering is changed at line 68 in PR1821 but because the variables are specified the new ordering works fine with the older ordering in my correction.

@arokem

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Hi @clintg6 : thanks for cleaning up after me! The truth is that I moved that key-word argument intentionally, because I wanted to give it a more prominent location, as it will often be required. This is really just a stylistic choice... Do you want to have a go at fixing the tutorials that were broken, instead of this fix?

@clintg6

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Hi @arokem I can fix the tutorials. Do you want me to issue a new PR when I'm done?

@arokem

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Yes. That would be great. Thanks!

@pep8speaks

This comment has been minimized.

Copy link

commented May 23, 2019

Hello @clintg6, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2019-05-24 20:30:56 UTC
@@ -42,7 +42,8 @@
"""

from dipy.segment.mask import median_otsu
stanford_b0_masked, stanford_b0_mask = median_otsu(data, median_radius=4, numpass=4)
stanford_b0_masked, stanford_b0_mask = median_otsu(data, median_radius=4,

This comment has been minimized.

Copy link
@arokem

arokem May 24, 2019

Member

This doesn't require a vol_idx input?

This comment has been minimized.

Copy link
@clintg6

clintg6 May 24, 2019

Author Contributor

No because the b0 itself is 3D. I accidentally introduced a typo. It should have been
stanford_b0_masked, stanford_b0_mask = median_otsu(stanford_b0, median_radius=4, numpass=4)
syn_b0_masked, syn_b0_mask = median_otsu(syn_b0, median_radius=4, numpass=4)
I have corrected it.

This comment has been minimized.

Copy link
@arokem

arokem May 24, 2019

Member

Gotcha. That makes sense!

@@ -41,8 +41,8 @@
from dipy.segment.mask import median_otsu


maskdata, mask = median_otsu(data, vol_idx=range(10, 50), median_radius=3, numpass=1,
autocrop=True, dilate=2)
maskdata, mask = median_otsu(data, vol_idx=range(10, 50), median_radius=3,

This comment has been minimized.

Copy link
@arokem

arokem May 24, 2019

Member

This is weird. I believe that the vol_idx here (and elsewhere using this data) should be just the values where the gtab.b0s_index is True. Or am I missing something?

This comment has been minimized.

Copy link
@clintg6

clintg6 May 24, 2019

Author Contributor

Yes, technically it should be where gtab.b0s_index is True if you want to perform skull stripping based upon the b0s. The skull is still present in the DWIs so I guess it still works with the range(10, 50).

@arokem

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Sorry, what is the issue here? Is vol_idx the default value to a range?

@arokem

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Nope this is the correct behavior here. Using a collection of non b0 volumes helps a lot the quality of segmentation. If you only use the b0 here this specific algorithm does not behave well. You would have to use a different dataset with a better b0 if you want to showcase vol_idx=0.

@arokem

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Agreed.

@arokem

This comment has been minimized.

Copy link
Member

commented May 24, 2019

In that case, I think that this is ready to be merged, right?

@Garyfallidis

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Are we good with pep8 everywhere?

@arokem

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Yes. Let's go!

@arokem arokem merged commit f41419d into nipy:master Jun 12, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@skoudoro skoudoro added this to the 1.0 milestone Jun 12, 2019

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