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

Add the mask workflow #1123

Merged
merged 7 commits into from Feb 17, 2017

Conversation

Projects
None yet
5 participants
@matthieudumont
Contributor

matthieudumont commented Sep 23, 2016

No description provided.

@coveralls

This comment has been minimized.

coveralls commented Sep 23, 2016

Coverage Status

Coverage decreased (-0.02%) to 83.064% when pulling f6b8c53 on matthieudumont:mask_workflow into d0bee8c on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 23, 2016

Coverage Status

Coverage decreased (-0.02%) to 83.064% when pulling f6b8c53 on matthieudumont:mask_workflow into d0bee8c on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Sep 23, 2016

Current coverage is 85.48% (diff: 93.18%)

Merging #1123 into master will increase coverage by 0.01%

@@             master      #1123   diff @@
==========================================
  Files           214        216     +2   
  Lines         24917      24961    +44   
  Methods           0          0          
  Messages          0          0          
  Branches       2526       2529     +3   
==========================================
+ Hits          21296      21338    +42   
- Misses         2989       2991     +2   
  Partials        632        632          

Powered by Codecov. Last update e87b327...70036e7

@arokem

This looks over all good. Please add a test here as well.

@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2016

Coverage Status

Coverage increased (+0.2%) to 83.252% when pulling a9bae1d on matthieudumont:mask_workflow into d0bee8c on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Oct 3, 2016

Coverage Status

Coverage increased (+0.2%) to 83.252% when pulling a9bae1d on matthieudumont:mask_workflow into d0bee8c on nipy:master.

@arokem

Great. I had just a couple of small comments, but overall looks good.

logging.info('Creating mask of {0}'
.format(input_path))
data, affine = load_nifti(input_path)
mask = np.bitwise_and(data > greater_than, data < less_than)

This comment has been minimized.

@arokem

arokem Oct 21, 2016

Member

Maybe error handling for the case where greater_than > less_than?

And testing for that case

This comment has been minimized.

@matthieudumont

matthieudumont Nov 22, 2016

Contributor

Good point, will do.

input_files : string
Path to image to be masked.
greater_than : float
Default is 0.2.

This comment has been minimized.

@arokem

arokem Oct 21, 2016

Member

What's the 0.2 default all about?

This comment has been minimized.

@matthieudumont

matthieudumont Nov 22, 2016

Contributor

I guess this should not be an optional param after all, no idea where 0.2 came from when I originally did this.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 1, 2016

@matthieudumont what is the purpose of this PR?

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Nov 1, 2016

@Garyfallidis : Add the mask workflow

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 1, 2016

@matthieudumont I noticed something while playing with the current master. In setup.py the script dipy_reconst_dti_restore is not available. The others are. You may want to add it here together with dipy_mask.

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Nov 1, 2016

Good catch, will add.

@arokem

This comment has been minimized.

Member

arokem commented Nov 1, 2016

And add a test that these things are installed and can run, please.

On Tue, Nov 1, 2016 at 12:28 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

@matthieudumont https://github.com/matthieudumont I noticed something
while playing with the current master. In setup.py the script
dipy_reconst_dti_restore is not available. The others are. You may want to
add it here together with dipy_mask.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1123 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHPNrb5iBT0vIGWY4rAP5A_G-4qk7tMks5q55L3gaJpZM4KFDDG
.

@matthieudumont matthieudumont force-pushed the matthieudumont:mask_workflow branch from a9bae1d to a2269f3 Nov 22, 2016

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Nov 22, 2016

About adding tests that all the available workflows are present and runable, I will make this another PR.

@coveralls

This comment has been minimized.

coveralls commented Nov 22, 2016

Coverage Status

Coverage increased (+0.01%) to 88.015% when pulling a2269f3 on matthieudumont:mask_workflow into e87b327 on nipy:master.

@@ -14,7 +14,7 @@ class MaskFlow(Workflow):
def get_short_name(cls):
return 'mask'
def run(self, input_files, greater_than=0.2, less_than=np.inf, out_dir='',
def run(self, input_files, greater_than, less_than=np.inf, out_dir='',

This comment has been minimized.

@arokem

arokem Nov 22, 2016

Member

How about ub and lb (for 'upper bound' and 'lower bound')?

@@ -214,6 +214,8 @@ def main(**extra_args):
data_files=[('share/doc/dipy/examples',
glob(pjoin('doc', 'examples','*.py')))],
scripts = [pjoin('bin', 'dipy_reconst_dti'),
pjoin('bin', 'dipy_reconst_dti_restore'),
pjoin('bin', 'dipy_mask'),

This comment has been minimized.

@arokem

arokem Nov 22, 2016

Member

This all seems pretty error prone. Is there some way to automate this, instead of having to add a line in here every time?

This comment has been minimized.

@matthieudumont

matthieudumont Nov 22, 2016

Contributor

I can look into that in the upcoming script tests PR.

@arokem

arokem approved these changes Nov 22, 2016

@coveralls

This comment has been minimized.

coveralls commented Nov 22, 2016

Coverage Status

Coverage increased (+0.01%) to 88.017% when pulling 70036e7 on matthieudumont:mask_workflow into e87b327 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Nov 22, 2016

Coverage Status

Coverage increased (+0.01%) to 88.017% when pulling 70036e7 on matthieudumont:mask_workflow into e87b327 on nipy:master.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 1, 2016

This looks good. Will merge in 2 days.

@matthieudumont

This comment has been minimized.

Contributor

matthieudumont commented Dec 12, 2016

bump! :)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 17, 2017

Sorry for the delay!

@Garyfallidis Garyfallidis merged commit 5b2fc62 into nipy:master Feb 17, 2017

4 checks passed

codecov/patch 93.18% of diff hit (target 85.46%)
Details
codecov/project 85.48% (+0.01%) compared to e87b327
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 88.017%
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

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