Adding niftyseg #1911

byvernault · 2017-03-29T13:46:31Z

Dear nipype team,

We are ready to add niftyseg to nipype. I copied all the interfaces we generated for this package as well as the test.

I tested it with python 2 and python 3. It's working on my computer.

Let me know if you see anything not compatible or that I should change.

Kind Regards,

Ben

codecov-io · 2017-03-29T17:51:54Z

Codecov Report

❗ No coverage uploaded for pull request base (master@38fa3d5). Click here to learn what that means.
The diff coverage is 62.45%.

@@            Coverage Diff            @@
##             master    #1911   +/-   ##
=========================================
  Coverage          ?   72.12%           
=========================================
  Files             ?     1115           
  Lines             ?    56246           
  Branches          ?     8083           
=========================================
  Hits              ?    40568           
  Misses            ?    14397           
  Partials          ?     1281

Flag	Coverage Δ
#smoketests	`72.12% <62.45%> (?)`
#unittests	`69.75% <62.45%> (?)`

Impacted Files	Coverage Δ
nipype/interfaces/setup.py	`6.66% <0%> (ø)`
...ype/interfaces/niftyseg/tests/test_label_fusion.py	`10.34% <10.34%> (ø)`
...rfaces/niftyseg/tests/test_auto_NiftySegCommand.py	`100% <100%> (ø)`
nipype/interfaces/niftyreg/tests/test_reg.py	`13.88% <100%> (ø)`
nipype/interfaces/niftyreg/__init__.py	`100% <100%> (ø)`
nipype/interfaces/niftyreg/tests/test_regutils.py	`4.56% <100%> (ø)`
nipype/interfaces/niftyseg/__init__.py	`100% <100%> (ø)`
nipype/interfaces/niftyseg/patchmatch.py	`100% <100%> (ø)`
nipype/interfaces/niftyseg/lesions.py	`100% <100%> (ø)`
nipype/interfaces/niftyseg/tests/test_maths.py	`12% <12%> (ø)`
... and 23 more

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 38fa3d5...3fb818f. Read the comment docs.

byvernault · 2017-04-04T15:41:32Z

Hi,

I think we are finally good with this branch to add niftyseg interface to nipype.

Let me know if you have any question.

Kind Regards,
Ben

oesteban · 2017-04-28T19:17:50Z

I'll look at this after #1913 is merged 👍

oesteban

This is pretty mature and almost ready for merge. Great job!

The only outstanding issue is: why not creating just one nifty package? Then, the niftyreg and niftyseg interfaces would be inside.

If we finally have two submodules (and not one, as I propose). I think it'd be important to remove nipype/interfaces/niftyseg/base.py and import everything from nipype.interfaces.niftyreg.base. All the duplicated code should be removed.

oesteban · 2017-04-28T22:47:17Z

nipype/interfaces/niftyseg/__init__.py

+Top-level namespace for niftyseg.
+"""
+
+from .base import no_niftyseg, get_custom_path


Are these used outside the module? If not, I'd remove them

hmmm I don't think so. I will remove them.

oesteban · 2017-04-29T03:21:19Z

nipype/interfaces/niftyseg/label_fusion.py

+            return None
+
+        if ranking == 'ALL':
+            return '-%s' % ranking


woudn't return '-ALL' be more readable?

yes indeed, we can do that.

oesteban · 2017-04-29T03:23:58Z

nipype/interfaces/niftyseg/em.py

+                    desc='4D file containing the priors',
+                    xor=['no_prior', 'priors'])
+
+    desc = 'List of priors filepaths.'


why not setting this string directly on the argument? (this holds for many traits defined below)

yes, it was for style and not go over 80 chars when they are very long but I should put this one back there as well as the other one.

oesteban · 2017-04-29T03:30:24Z

nipype/interfaces/niftyseg/label_fusion.py

+            try:
+                out_files = load_json(outfile)['files']
+            except IOError:
+                return self.run().outputs


I am doubtful of this self.run(), WDYT @satra?

oesteban · 2017-04-29T03:33:51Z

nipype/interfaces/niftyseg/label_fusion.py

+        outputs = self._outputs()
+        # local caching for backward compatibility
+        outfile = os.path.join(os.getcwd(), 'CalcTopNCC.json')
+        if runtime is None:


Wouldn't be better to check for non-empty runtime.stdout as well?:

if runtime is None or not runtime.stdout: ...

sure. Thanks.

oesteban · 2017-04-29T03:37:11Z

nipype/interfaces/niftyseg/maths.py

+    _dtypes = ['float', 'char', 'int', 'short', 'double', 'input']
+
+    desc = 'datatype to use for output (default uses input type)'
+    output_datatype = traits.Enum(*_dtypes,


This is totally stylistic, but _dtypes is not used anymore right?, why not having the options in the Enum initialization?

oesteban · 2017-04-29T03:44:00Z

nipype/interfaces/niftyseg/stats.py

+    >>> node = niftyseg.UnaryStats()
+    >>> node.inputs.in_file = 'im1.nii'
+    >>> node.inputs.operation = 'v'
+    >>> node.cmdline  # doctest: +ALLOW_UNICODE


I think all these interfaces would deserve some more doctests with more operations, so that we are sure that they work and that we don't break them in future maintenance.

oesteban · 2017-04-29T03:44:42Z

nipype/interfaces/niftyseg/maths.py

+    >>> node.inputs.operand_file = 'im2.nii'
+    >>> node.inputs.output_datatype = 'float'
+    >>> node.cmdline  # doctest: +ALLOW_UNICODE
+    'seg_maths im1.nii -sub im2.nii -odt float im1_sub.nii'


I think all these interfaces would deserve some more doctests with more operations, so that we are sure that they work and that we don't break them in future maintenance.

Do you think I can follow fslmaths doctests to edit those ones?

What I had in mind is something closer to the documentation of nipype.interfaces.ants.registration.Registration: https://github.com/nipy/nipype/blob/master/nipype/interfaces/ants/registration.py#L432-L640

Here, you'll see how the inputs to the interface are varied and then the new cmdline tested.

I can do that but for those interfaces it's pretty easy, only the operation changed. It's kind of the same than fslmaths.

I added different exemples for some operation but by doing so I realised that I am not checking the inputs for some argument (for example pow, thr, uthr, ... use only a float or min uses only a file). I am adding that to the interface.

Let me know if you approve this and you really want to see one test per operation.

oesteban · 2017-04-29T03:46:52Z

nipype/interfaces/setup.py

@@ -24,6 +24,7 @@ def configuration(parent_package='', top_path=None):
    config.add_subpackage('mne')
    config.add_subpackage('mrtrix')
    config.add_subpackage('mrtrix3')
+    config.add_subpackage('niftyseg')


I would recommend having only one subpackage. I'll comment on this on the summary.

Niftyreg/Niftyseg/Niftyfit are different packages that you install separately. Do you think having only one package won't be odd for the user? What do you think?

I could remove probably the base and use the same commands from niftyreg.base in this package. Or do you mean moving all the code into the same folder?

Ok, I don't have an special interest for having only one package. Let's observe that preference (reg/seg/fit packages) 👍 .

So if we keep the 3 packages structure, then I'm suggesting here to import from niftyreg.base and remove the duplicated code from niftyseg and niftyfit.

WDYT?

Yes sounds good. I jut did the changes and I renamed the function in niftyreg base for something more general like no_nifty_package.

What do you think about the changes?

Also do you prefer to import NiftyRegCommand like this:

import ..niftyreg.base.NiftyRegCommand as NiftySegCommand

Or do as I did?

I will do then the edits on niftyfit when you agree with those changes.

Kind Regards,
Ben

Oh there are some issues for the test since most executables in niftyseg don't have a --version options that is working.

I will work on fixing this later today.

I add back the base.py just to create NiftySegCommand because of the version that isn't available for all the executables.

I also edited slightly the base.py for NiftyRegCommand.
The test should pass now.

Let me know what you think.

Cheers,
Ben

…ions as well

… the software.

byvernault added 3 commits March 28, 2017 12:58

Inital commit to add niftyseg

269987d

Adding db.xml to data for testing purpose.

013bf72

Converting files to python2/3 compatibility

f431e0c

byvernault added 3 commits March 31, 2017 15:58

Editing docstring to follow nipype template and add some tests.

c7e43ea

Adding to cmdline # doctest: +ELLIPSIS +ALLOW_UNICODE

135da56

Fixing the docstring

b9610cb

byvernault added 6 commits April 20, 2017 09:50

adding documentation in docstring for segmaths, segstats, seglabfusion

55e555e

Symplifying the interfaces when possible using name_source/name_template

ee44a57

renaming test to create test auto

3ab6250

fixing doctests

84adfcd

Using overload_extension instead of list_output/gen_filename

23dccd8

Editing few options on the interface FillLesions

be7c489

mgxd added the needs-review label Apr 26, 2017

Merge branch 'master' into adding_niftyseg

ef14ef5

oesteban requested changes Apr 29, 2017

View reviewed changes

byvernault added 5 commits May 1, 2017 17:35

removing base.py and using niftyreg base.py class/fct. Renaming funct…

c40b673

…ions as well

Adding back base.py for NiftySegCommand because of version control on…

3389a28

… the software.

adding more tests in docstring for seg_maths and seg_stats

cf30734

fixing doctest

617eb48

few more errors in doctest to fix

3fb818f

oesteban approved these changes May 3, 2017

View reviewed changes

satra merged commit a4cc711 into nipy:master May 3, 2017

Adding niftyseg #1911

Adding niftyseg #1911

Uh oh!

Conversation

byvernault commented Mar 29, 2017

Uh oh!

codecov-io commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

byvernault commented Apr 4, 2017

Uh oh!

oesteban commented Apr 28, 2017

Uh oh!

oesteban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byvernault May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byvernault May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Mar 29, 2017 •

edited

Loading

byvernault May 1, 2017 •

edited

Loading

byvernault May 2, 2017 •

edited

Loading