Skip to content

FIX: Ensure outputs can be listed in camino.ProcStreamlines by defining instance variable #2739

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 1 commit into from
Jan 30, 2019

Conversation

cni-md
Copy link
Contributor

@cni-md cni-md commented Oct 19, 2018

Summary

Camino procstreamlines can be used without -outputroot to filter streamlines. The function _list_outputs insits on a definition of outputroot_files in contrast to _run_interface.

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@@ -463,7 +463,8 @@ def _get_actual_outputroot(self, outputroot):
def _list_outputs(self):
outputs = self.output_spec().get()
outputs['proc'] = os.path.abspath(self._gen_outfilename())
outputs['outputroot_files'] = self.outputroot_files
if isdefined(self.inputs.outputroot):
outputs['outputroot_files'] = self.outputroot_files
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a slightly bigger bug, which is that self.outputroot_files is not defined until the interface is run, and then only sometimes. I would instead make sure that it is defined at __init__:

def __init__(self, *args, **kwargs):
    super(ProcStreamlines, self).__init__(*args, **kwargs)
    self.outputroot_files = []

This way _list_outputs() will not raise an exception either before or after the interface is run, and whether self.inputs.outputroot is defined or not. For OutputMultiPaths, [] is equivalent to Undefined, so this change can be reverted, as well.

@effigies
Copy link
Member

Hi @cni-md. Do you have a few minutes to finish this one off?

@cni-md
Copy link
Contributor Author

cni-md commented Oct 25, 2018

@effigies not this month.

@effigies effigies added this to the 1.1.5 milestone Oct 25, 2018
@effigies
Copy link
Member

When you do get back to this, please merge master to fix tests.

@effigies effigies modified the milestones: 1.1.5, 1.1.6 Nov 7, 2018
@effigies
Copy link
Member

Just a heads up: We're looking to make the next release on Nov 26, if you want to try to get this in this month.

@effigies effigies modified the milestones: 1.1.6, 1.1.7 Nov 26, 2018
@effigies effigies modified the milestones: 1.1.7, 1.1.8 Dec 14, 2018
@effigies
Copy link
Member

Hi, just a reminder that the 1.1.8 release is targeted for January 28. Please let us know if you'd like to try to finish this up for that release.

@effigies effigies mentioned this pull request Jan 25, 2019
16 tasks
@cni-md
Copy link
Contributor Author

cni-md commented Jan 25, 2019

@effigies Ups, I had to work on other projects first. I am not able to finish it til monday 28th. Let's keep it for 1.1.9

@effigies
Copy link
Member

@cni-md Thanks for the response! I'll push to 1.1.9.

@effigies effigies modified the milestones: 1.1.8, 1.1.9 Jan 25, 2019
@codecov-io
Copy link

codecov-io commented Jan 30, 2019

Codecov Report

Merging #2739 into master will decrease coverage by 0.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2739      +/-   ##
==========================================
- Coverage   67.49%   67.12%   -0.37%     
==========================================
  Files         342      342              
  Lines       43559    43562       +3     
  Branches     5422     5422              
==========================================
- Hits        29399    29240     -159     
- Misses      13458    13575     +117     
- Partials      702      747      +45
Flag Coverage Δ
#smoketests 49.14% <ø> (-1.35%) ⬇️
#unittests 64.92% <100%> (ø) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/camino/convert.py 72.03% <100%> (+0.32%) ⬆️
nipype/workflows/fmri/fsl/preprocess.py 72.67% <0%> (-13.39%) ⬇️
nipype/utils/config.py 61.82% <0%> (-4.31%) ⬇️
nipype/pipeline/engine/utils.py 78.82% <0%> (-3.44%) ⬇️
nipype/workflows/fmri/fsl/estimate.py 61.97% <0%> (-2.82%) ⬇️
nipype/interfaces/fsl/model.py 77.91% <0%> (-2.71%) ⬇️
nipype/interfaces/fsl/preprocess.py 80.33% <0%> (-2.27%) ⬇️
nipype/pipeline/plugins/multiproc.py 82.71% <0%> (-1.86%) ⬇️
nipype/interfaces/io.py 53.73% <0%> (-1.34%) ⬇️
nipype/interfaces/fsl/base.py 86.66% <0%> (-1.12%) ⬇️
... and 8 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 ce65afb...ca3cf74. Read the comment docs.

@cni-md
Copy link
Contributor Author

cni-md commented Jan 30, 2019

@effigies I changed the file as you suggested. It is still working, with or without outputroot.
ca3cf74

@effigies effigies changed the title camino: fix procstreamline raw streamline output, without outputroot FIX: Ensure outputs can be listed in camino.ProcStreamlines by defining instance variable Jan 30, 2019
@effigies
Copy link
Member

LGTM, thanks.

@effigies effigies merged commit 86a3ea3 into nipy:master Jan 30, 2019
yarikoptic added a commit to yarikoptic/nipype that referenced this pull request Mar 15, 2019
1.1.9 (February 25, 2019)

Full changelog: https://github.com/nipy/nipype/milestone/30?closed=1

  * FIX: Make positional arguments to LaplacianThickness require previous argument (nipy#2848)
  * FIX: Import math and csv modules for bids_gen_info (nipy#2881)
  * FIX: Ensure outputs can be listed in camino.ProcStreamlines by defining instance variable (nipy#2739)
  * ENH: Allow afni.MaskTool to take multiple input files (nipy#2892)
  * ENH: Add flags dictionary input to spm.Level1Design (nipy#2861)
  * ENH: Threshold stddev once only in TSNR (nipy#2883)
  * ENH: Add workbench.CiftiSmooth interface (nipy#2871)
  * DOC: Replace initialism typo in comment with intended phrase (nipy#2875)
  * DOC: Fix typos in ANTs Registration input documentation (nipy#2869)

* tag '1.1.9': (34 commits)
  MNT: Update changelog
  MNT: Add Katherine Bottenhorn, Paul Mihai to Zenodo
  MNT: Add kchawla-pi to Zenodo, update mailmap and ordering
  add to zenodo
  MNT: Update zenodo ordering
  Update .zenodo.json
  afni utils.py - masktool - InputMultiPath for in_file argument
  MNT: Update .zenodo ordering
  MNT: Add Oliver Contier name to .zenodo.json
  Update nipype/interfaces/spm/model.py
  ENH: Add zenodo updating script
  MNT: Update mailmap to avoid renames in script
  MNT: Update .mailmap, .zenodo.json
  MNT: Version 1.1.9
  DOC: 1.1.9 changelog
  ENH: minor - compute non degenerate stddev map once
  BF: regenerated test_auto_LaplacianThickness using wonderfully long running tools/checkspecs.py
  TEST: Thorough test of LaplacianThickness requirement cascade
  FIX: Requires error text was backwards
  import math and csv modules for bids_gen_info
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants