Skip to content

ENH: add interface for MRTrix3's dwidenoise #2568

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

Merged
merged 5 commits into from
May 19, 2018
Merged

Conversation

mvdoc
Copy link
Member

@mvdoc mvdoc commented May 7, 2018

Changes proposed in this pull request

@mvdoc
Copy link
Member Author

mvdoc commented May 7, 2018

Apparently there's a timeout kicking in when downloading the test data

curl: (7) Failed to connect to storage.googleapis.com port 443: Connection timed out

@effigies
Copy link
Member

effigies commented May 7, 2018

I'm seeing similar problems for pip installations. Possibly Circle is getting rate-limited by some recent web server release (I've been able to navigate to the timing-out locations manually). Hopefully it's a temporary condition.

@mvdoc
Copy link
Member Author

mvdoc commented May 10, 2018

@effigies do you want me to push a commit to start the ci testing?

@effigies
Copy link
Member

Sure. Maybe just merge master, to avoid having a pointless commit?

* master:
  workaround to ICA-AROMA chdir
@codecov-io
Copy link

codecov-io commented May 10, 2018

Codecov Report

Merging #2568 into master will increase coverage by 0.55%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2568      +/-   ##
==========================================
+ Coverage   67.13%   67.69%   +0.55%     
==========================================
  Files         333      335       +2     
  Lines       42547    42822     +275     
  Branches     5266     5351      +85     
==========================================
+ Hits        28566    28990     +424     
+ Misses      13279    13153     -126     
+ Partials      702      679      -23
Flag Coverage Δ
#smoketests 50.7% <ø> (ø) ⬆️
#unittests 65.18% <100%> (+0.7%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/mrtrix3/preprocess.py 79.61% <100%> (+2.41%) ⬆️
nipype/interfaces/mrtrix3/__init__.py 100% <100%> (ø) ⬆️
nipype/interfaces/fsl/utils.py 70.62% <0%> (ø) ⬆️
nipype/algorithms/icc.py 59.21% <0%> (ø) ⬆️
nipype/utils/docparse.py 53.04% <0%> (ø) ⬆️
nipype/interfaces/mixins/__init__.py 100% <0%> (ø)
nipype/interfaces/mixins/reporting.py 48.57% <0%> (ø)
nipype/interfaces/fsl/model.py 80.61% <0%> (+0.18%) ⬆️
nipype/interfaces/afni/preprocess.py 81.5% <0%> (+0.24%) ⬆️
... and 29 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 e26e736...3e33f0d. Read the comment docs.

@mvdoc
Copy link
Member Author

mvdoc commented May 11, 2018

Seems it's failing still for timeout, but for another reason:

180511-00:41:51,523 workflow INFO:
         [Node] Finished "level1.datasink".
180511-00:41:53,304 workflow DEBUG:
         Progress: 52 jobs, 50/2/0 (done/running/ready), 2/0 (pending_tasks/waiting).
180511-00:41:53,304 workflow INFO:
         [Job 45] Completed (level1.datasink).
180511-00:41:53,307 workflow DEBUG:
         Tasks currently running: 1. Pending: 1.
180511-00:41:53,308 workflow INFO:
         [MultiProc] Running 1 tasks, and 0 jobs ready. Free memory (GB): 6.37/6.57, Free processors: 1/2.
                     Currently running:
                       * level1.report
180511-00:41:55,306 workflow DEBUG:
         Progress: 52 jobs, 51/1/0 (done/running/ready), 1/0 (pending_tasks/waiting).
180511-00:41:55,306 workflow DEBUG:
         Tasks currently running: 1. Pending: 1.
Too long with no output (exceeded 1h0m0s)

@effigies
Copy link
Member

This happens with that test, sometimes. I think it's related to #2548.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Looks good. A couple minor suggestions.

exists=True,
argstr='-mask %s',
position=1,
mandatory=False,
Copy link
Member

Choose a reason for hiding this comment

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

We don't generally set mandatory=True or exists=False.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant mandatory=False. I fixed the change here.

usedefault=True,
argstr="%s",
position=-1,
desc="the output denoised DWI image")
Copy link
Member

Choose a reason for hiding this comment

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

Consider using the following:

out_file = File(
    name_template='%s_denoised',
    name_source='in_file', 
    keep_extension=True,
    argstr="%s",
    position=-1,
    desc="the output denoised DWI image")

This will also auto-fill out_file in the output spec, so you can drop _list_outputs.

@effigies
Copy link
Member

@mvdoc Just a bump. Do you have a couple minutes to finish this one up?

@mvdoc
Copy link
Member Author

mvdoc commented May 18, 2018 via email

@effigies effigies added this to the 1.0.4 milestone May 18, 2018
@effigies effigies merged commit caf87f5 into nipy:master May 19, 2018
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.

3 participants