WIP: Dtitk #2185

kesshijordan · 2017-09-14T18:14:59Z

Changes proposed in this pull request

Added DTITK interfaces (http://dti-tk.sourceforge.net/pmwiki/pmwiki.php)

Note: make check-before-commit is throwing an error (see below) I am having trouble figuring out. If anyone has suggestions, please let me know.

Thanks!

Kesshi

____ ERROR collecting nipype/interfaces/dtitk/tests/test_auto_AffineTask.py ____
nipype/interfaces/dtitk/tests/test_auto_AffineTask.py:3: in
from ..registration import AffineTask
E ValueError: attempted relative import beyond top-level package
____ ERROR collecting

mgxd · 2017-09-15T17:05:27Z

@kesshijordan

Your travis build is failing because the doctests are actually being run - however dtitk isn't present in the environment. You can just add # doctest: +SKIP to the line in your doctest that has either node.run() or node.cmdline
As for circle, could you try merging with current nipype master?

kesshijordan · 2017-09-18T19:28:02Z

@mgxd I'm not sure how to debug this; if you have any suggestions they are most welcome.

I merged with the current nipype master, but am getting the error, below, in the circleci.

echo 'These tests were designed to be run at 4x parallelism.'
These tests were designed to be run at 4x parallelism.

Are you familiar with the error, below? It's the same one I get when I try make check-before-commit and was having trouble figuring out.

Thanks!
Kesshi

____ ERROR collecting nipype/interfaces/dtitk/tests/test_auto_AffineTask.py ____
nipype/interfaces/dtitk/tests/test_auto_AffineTask.py:3: in
from ..registration import AffineTask
E ValueError: Attempted relative import in non-package

mgxd · 2017-09-18T21:52:21Z

@kesshijordan

regarding travis - you're missing an __init__.py in your dtitk/tests directory.

for circle - did you start working on this branch awhile ago? It seems your circle build is not running on 4x parallelism, you may have to manually change that within your circleci build settings. Perhaps @oesteban can chime in here

oesteban · 2017-09-18T22:59:27Z

Yes - for circle just go to https://circleci.com/gh/kesshijordan/nipype/edit#parallel-builds and set 4x for your builds.

codecov-io · 2017-09-19T05:21:16Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #2185   +/-   ##
=========================================
  Coverage          ?   72.41%           
=========================================
  Files             ?     1210           
  Lines             ?    59762           
  Branches          ?     8595           
=========================================
  Hits              ?    43274           
  Misses            ?    15090           
  Partials          ?     1398

Flag	Coverage Δ
#smoketests	`72.41% <76.21%> (?)`
#unittests	`70.01% <76.21%> (?)`

Impacted Files	Coverage Δ
...rfaces/dtitk/tests/test_auto_TVAdjustOriginTask.py	`100% <100%> (ø)`
...erfaces/dtitk/tests/test_auto_TVAdjustVoxSpTask.py	`100% <100%> (ø)`
nipype/interfaces/dtitk/__init__.py	`100% <100%> (ø)`
...terfaces/dtitk/tests/test_auto_CommandLineDtitk.py	`100% <100%> (ø)`
...pype/interfaces/tests/test_auto_SimpleInterface.py	`100% <100%> (ø)`
nipype/interfaces/dtitk/base.py	`38.46% <38.46%> (ø)`
nipype/interfaces/dtitk/utils.py	`60.73% <60.73%> (ø)`
nipype/interfaces/dtitk/registration.py	`80% <80%> (ø)`
...terfaces/dtitk/tests/test_auto_affScalarVolTask.py	`85.71% <85.71%> (ø)`
...ype/interfaces/dtitk/tests/test_auto_DiffeoTask.py	`85.71% <85.71%> (ø)`
... and 12 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 157d3bd...f60c149. Read the comment docs.

mgxd · 2017-09-19T15:02:30Z

@kesshijordan you are close to passing the checks - you'll just have to run make specs and add the updated autotests

kesshijordan · 2017-09-19T16:02:16Z

thank you @mgxd and @oesteban!

mgxd

A few comments - note that I haven't actually tested the functions though

mgxd · 2017-09-20T15:47:16Z

.cache/v/cache/lastfailed

@@ -0,0 +1,22 @@
+{


this file isn't necessary

Two quick questions:

There isn't anything in the directories from .cache on: Is there any reason not to remove everything including .cache? (or is it better to just delete lastfailed?)

I see the comment about lastfailed not being necessary: is that the only comment, or is there somewhere else I should be looking for other comments?

Thanks again, @mgxd!
Kesshi

@kesshijordan

I believe the .cache directory is created by pytest when running nipype's tests, and this file just serves as a reminder when retrying - you can keep this in your local clone, but it doesn't really serve a purpose on the master repo.

Nope, that's it!

@kesshijordan - lastfailed is the only one that's included in the commit. probably got added by mistake. but in general nothing in .cache should be in the commit.

Thanks, @satra. I removed the .cache file from git and have pushed the change. @mgxd is there anything else I need to address?

satra · 2017-10-03T16:21:12Z

@kesshijordan and @mgxd - could this be reviewed/updated soon? i think it's very close.

mgxd · 2017-09-20T15:47:29Z

nipype/interfaces/dtitk/__init__.py

+"""
+
+# from .base import ()
+from ..dtitk.registration import (RigidTask, AffineTask, DiffeoTask,


from .registration import ... is a bit cleaner

mgxd · 2017-09-20T15:47:34Z

nipype/interfaces/dtitk/__init__.py

+                                  ComposeXfmTask, diffeoSymTensor3DVolTask,
+                                  affSymTensor3DVolTask, affScalarVolTask,
+                                  diffeoScalarVolTask)
+from ..dtitk.utils import (TVAdjustOriginTask, TVAdjustVoxSpTask,


same as above

mgxd · 2017-09-20T15:48:17Z

nipype/interfaces/dtitk/registration.py

+
+    def _list_outputs(self):
+        outputs = self.output_spec().get()
+        outputs['out_file_xfm'] = self.inputs.in_file.replace('.nii.gz',


this may be a good time to use _gen_fname()

mgxd · 2017-09-20T15:48:56Z

nipype/interfaces/dtitk/registration.py

+
+    def _list_outputs(self):
+        outputs = self.output_spec().get()
+        outputs['out_file_xfm'] = self.inputs.in_fixed_tensor.replace(


same as above

mgxd · 2017-09-20T15:50:09Z

nipype/interfaces/dtitk/utils.py

+        if not isdefined(self.inputs.out_file):
+            outputs["out_file"] = self._gen_fname(self.inputs.in_file,
+                                                  suffix=self._suffix,
+                                                  ext='.'+'.'.join(


will this cover .nii files too?

mgxd · 2017-09-21T14:50:46Z

.cache/v/cache/lastfailed

@@ -0,0 +1,22 @@
+{


@kesshijordan

I believe the .cache directory is created by pytest when running nipype's tests, and this file just serves as a reminder when retrying - you can keep this in your local clone, but it doesn't really serve a purpose on the master repo.

Nope, that's it!

…ng that

satra · 2017-10-16T01:04:44Z

is this still a wip, or is this ready for merge?

kesshijordan · 2017-10-16T18:35:00Z

Still a WIP... I need to test the way I addressed the last few comments to make sure I don't mess anything up naming the outputs.

satra · 2017-10-24T15:30:17Z

@kesshijordan - any chance we can get this in by the oct 31 deadline? we are going to cut a new release.

kesshijordan · 2017-10-25T00:10:13Z

👍

satra · 2017-10-29T21:47:52Z

@kesshijordan - a recent PR from @oesteban takes care of the unicode issue. could you please make sure you are merged with current master?

oesteban · 2017-10-30T19:00:00Z

@satra I think that PR did not affect the 'stream' type of terminal_output.

@kesshijordan, in order to facilitate your debugging, could you set terminal_output = 'file_split' on your interface? That would avoid the problem you are getting (after you merge current master as Satra suggested).

I have opened an issue (link is above) for the UnicodeDecodeError problem.

kesshijordan · 2017-10-30T20:51:13Z

Thanks, @oesteban! I set it when I use the interface (like the example below), and it will run without the UTF-8 error. This doesn't interfere with my debugging, but FYI: if I run it such that the command line would throw and error, the node runs until manually killed with control C. When you kill it and open up the stderr.nipype, the node appears to be running over and over with jumbled letters replacing the filenames.

https://github.com/nipy/nipype/blob/1a7401328baf4aca0f163ca18d3a88b913412eb3/doc/users/saving_workflows.rst

kesshijordan · 2017-10-30T21:45:56Z

Thanks for the pointers, @mgxd. I tried your suggestion for installing and it worked... this is much easier! Regarding question 2: the error occurred because I hadn't set some of the inputs the command line needed; I am using python 3 (though when I first ran this, I think I was using 2.7). I got the rigid registration interface to work and will modify the subsequent interfaces in the same way so they take two niftis and a set of parameters instead of a nifti and a text file containing a nifti (different functions in dti-tk). I'm having trouble getting the generalized file naming to work based on other examples (I keep getting errors that it isn't a "file name" when I manipulate the string) so right now I'm just leaving it at only supporting .nii.gz so I can try to get the rest done before the deadline tomorrow.

mgxd

@kesshijordan we can support nii.gz for now, make sure to merge to upstream and remove the extra files and this should be merged soon

mgxd · 2018-01-05T21:13:39Z

stderr.nipype

@@ -0,0 +1,3 @@
+/Users/kesshijordan/repos/dtitk-2.3.3-Darwin-x86_64/scripts/dti_diffeomorphic_reg: line 191: [: 1.0: integer expression expected


you can remove this

mgxd · 2018-01-05T21:13:49Z

stdout.nipype

@@ -0,0 +1,3 @@
+registering /Users/kesshijordan/ref_data/interscan_dys/Trio/IC004-1_dti_b2000_EC_tensorx1000_aff.nii.gz to /Users/kesshijordan/ref_data/interscan_dys/Prisma/IC004-4_dti_b2500_SC_EC_tensorx1000.nii.gz ...


Also remove

mgxd · 2018-01-05T21:15:44Z

nipype/interfaces/tests/test_auto_SimpleInterface.py

@@ -0,0 +1,16 @@
+# AUTO-GENERATED by tools/checkspecs.py - DO NOT EDIT


remove this too

effigies · 2018-01-19T15:24:58Z

Is this something we want in for 1.0? I merged master and updated the auto tests.

effigies · 2018-01-20T22:50:44Z

Tests are passing. Is this good to merge? @mgxd @kesshijordan

mgxd · 2018-01-21T03:46:36Z

we will probably need to re-add the yapf stylized files after make specs - if they are modified I'll add them in a general housekeeping PR before 1.0

Kesshi jordan and others added 7 commits May 19, 2017 14:29

added interface for dtitk

6ea8e5d

Added interface for DTITK command-line tools

5232552

Added several docstring tests and untested interfaces

ae59226

changed relative imports to absolute, fixed docstring tests

f1dc18b

Merge remote-tracking branch 'upstream/master' into dtitk

8b45b9b

added init

c7122ac

changed input variable names

7b8b5db

kesshijordan added 2 commits September 15, 2017 10:27

added # doctest: +SKIP

1127b4d

Merge remote-tracking branch 'upstream/master' into dtitk

3808fee

kesshijordan added 2 commits September 18, 2017 17:32

added init file

54d9d1d

fixed variable name changes

0a4bda2

added auto-generated tests

b37f65f

mgxd reviewed Sep 20, 2017

View reviewed changes

removed .cache from git

3182baa

mgxd reviewed Oct 3, 2017

View reviewed changes

kesshijordan added 2 commits October 13, 2017 14:16

Merge branch 'master' of https://github.com/nipy/nipype into dtitk

7eaab1c

fixed some changes from mgxd code review, but circle failing so testi…

8812dc7

…ng that

satra added this to the 0.14.0 milestone Oct 29, 2017

oesteban mentioned this pull request Oct 30, 2017

UnicodeDecodeError: 'utf-8' codec can't decode - stream output #2265

Open

kesshijordan added 3 commits October 30, 2017 12:00

testing file naming

6a393ca

Merge remote-tracking branch 'upstream/master' into dtitk

af96a0b

Rigid registration node tested and works

d240443

tested affine

0807cdb

mgxd modified the milestones: 0.14.0, 0.14.1 Nov 15, 2017

kesshijordan and others added 6 commits December 1, 2017 18:04

returned to .nii.gz; will deal with .nii later

78821e4

changed output var names

948811f

fixed typo

89628d0

fix typo

9c84405

regenerated tests so var names are updated

7ad09b8

Merge branch 'master' into dtitk

74f736e

mgxd approved these changes Jan 5, 2018

View reviewed changes

kesshijordan and others added 5 commits January 11, 2018 13:38

Delete stderr.nipype

64f7202

Delete stdout.nipype

80a4262

Merge remote-tracking branch 'upstream/master' into dtitk

b567e0a

make specs

da31236

TEST: Drop extra auto test

f60c149

mgxd merged commit 1c174df into nipy:master Jan 21, 2018

mgxd modified the milestones: 0.14.1, 1.0 Jan 21, 2018

		@@ -0,0 +1,3 @@
		/Users/kesshijordan/repos/dtitk-2.3.3-Darwin-x86_64/scripts/dti_diffeomorphic_reg: line 191: [: 1.0: integer expression expected

		@@ -0,0 +1,3 @@
		registering /Users/kesshijordan/ref_data/interscan_dys/Trio/IC004-1_dti_b2000_EC_tensorx1000_aff.nii.gz to /Users/kesshijordan/ref_data/interscan_dys/Prisma/IC004-4_dti_b2500_SC_EC_tensorx1000.nii.gz ...

		@@ -0,0 +1,16 @@
		# AUTO-GENERATED by tools/checkspecs.py - DO NOT EDIT

WIP: Dtitk #2185

WIP: Dtitk #2185

Conversation

kesshijordan commented Sep 14, 2017

mgxd commented Sep 15, 2017

kesshijordan commented Sep 18, 2017

mgxd commented Sep 18, 2017

oesteban commented Sep 18, 2017

codecov-io commented Sep 19, 2017 • edited Loading

Codecov Report

mgxd commented Sep 19, 2017

kesshijordan commented Sep 19, 2017

mgxd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satra commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satra commented Oct 16, 2017

kesshijordan commented Oct 16, 2017

satra commented Oct 24, 2017

kesshijordan commented Oct 25, 2017

satra commented Oct 29, 2017

oesteban commented Oct 30, 2017

kesshijordan commented Oct 30, 2017

kesshijordan commented Oct 30, 2017 • edited Loading

mgxd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Jan 19, 2018

effigies commented Jan 20, 2018

mgxd commented Jan 21, 2018

codecov-io commented Sep 19, 2017 •

edited

Loading

kesshijordan commented Oct 30, 2017 •

edited

Loading