Skip to content

FIX: some fixes to afni.Deconvolve #2334

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jan 20, 2018
Merged

FIX: some fixes to afni.Deconvolve #2334

merged 12 commits into from
Jan 20, 2018

Conversation

mvdoc
Copy link
Member

@mvdoc mvdoc commented Dec 10, 2017 •
edited
Loading

  1. If 3dDeconvolve is run with --x1D_stop, the output file is not created. The interface currently fails because it assumes that the output file exists. This PR checks whether x1D stop is set, and changes the exists flag of output_spec.out_file.
  2. force_TR should be float not int and should precede -input
  3. fix formatting of ortvec

@mvdoc mvdoc changed the title FIX: if x1D_stop in Deconvolve, do not assume that out_file exists FIX: some fixes to afni.Deconvolve Dec 10, 2017
@satra satra added this to the 0.14.1 milestone Dec 10, 2017
@mvdoc
Copy link
Member Author

mvdoc commented Dec 11, 2017

My attempt to dynamically change the exists flag of out_file didn't really work, so I set exists to False. But if you have any suggestion on how I can modify dynamically a trait in output spec, I feel that would be better.

@mgxd
Copy link
Member

mgxd commented Jan 8, 2018 •
edited
Loading

@mvdoc how about leaving exists=False within the outputspec, but removing out_file from outputs in Deconvolve._list_outputs if x1d_stop is set to True?

mvdoc added 2 commits January 18, 2018 18:14
@mvdoc
Merge remote-tracking branch 'origin/master' into afni/deconvolve
6ad3247 
* origin/master: (139 commits)
  fix error type
  remove install_aliases
  sty
  fix tests
  refactoring tests
  update changes
  [MAINT] Cleanup engine/base
  STY: Add newline
  pep
  formatting
  Revert "after make specs"
  address review and add changelog
  MAINT: Adjust test names to placate make specs
  Fix 3dFWHMx outputs
  fix: composemultitransform doctest
  sty: fix examples
  sty: fix styles in updated interfaces
  sty: cleanup tools folder
  sty: add yapf to extras installs
  fix: specs
  ...
@mvdoc
Remove out_file if x1d_stop set to True
c4d6e31
@mvdoc
Copy link
Member Author

mvdoc commented Jan 18, 2018

Done @mgxd. Let me know. Pinging @effigies as well.

effigies
effigies requested changes Jan 19, 2018
View reviewed changes
nipype/interfaces/afni/tests/test_deconvolve.py Outdated
@@ -0,0 +1,10 @@
"""Test afni deconvolve"""
Copy link
Member

Choose a reason for hiding this comment

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

Please rename this file to "test_extra_Deconvolve".

This isn't yet a part of documentation, but we realized that lowercase tests like this block building the auto test on OSX. Using "extra" marks it as something to have in addition to the auto-test. And just for consistency's sake, let's use the capitals.

@effigies
Copy link
Member

Also this needs a make specs update.

@mvdoc
Copy link
Member Author

mvdoc commented Jan 19, 2018

@effigies: done

effigies
effigies approved these changes Jan 19, 2018
View reviewed changes
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. Will merge on passing tests.

@effigies effigies modified the milestones: 0.14.1, 1.0 Jan 19, 2018
@effigies effigies merged commit 04d8e0e into nipy:master Jan 20, 2018
@mvdoc mvdoc mentioned this pull request Jan 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@effigies effigies effigies approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mvdoc @mgxd @effigies @satra