Skip to content
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

[ENH] Add afni.LocalStat and afni.ReHo, update afni.ROIStats inputs #2740

Merged
merged 14 commits into from
Oct 30, 2018

Conversation

rciric
Copy link
Contributor

@rciric rciric commented Oct 20, 2018

Summary

  • A new interface for 3dLocalstat adds support for voxelwise (local) confounds, for use in ANATICOR-like denoising protocols.
  • A new interface for 3dReHo enables estimation of voxelwise and ROI-wise regional homogeneity.
  • The interface for 3dROIstats has been reworked to support a broader range of parameters and regional statistics.

List of changes proposed in this PR (pull-request)

  • Localstat interface added for AFNI's 3dLocalstat.
  • ReHo interface added for AFNI's 3dReHo.
  • ROIstats input spec accepts new parameters: num_roi, zerofill, roisel, debug, nomeanout, nobriklab, format1D, format1DR, and stat. This means that parameters to the ROIstats interface can now be used to specify computation of regional statistics other than the default zero-inclusive mean.
  • The changes to ROIstats are not backward-compatible; for instance, the output has been renamed from stats to out_file. Software using the existing ROIstats interface will need to be updated accordingly.

Questions

  • Is backward compatibility or internal naming consistency generally more important? The changes to ROIstats currently favour internal consistency, but it would be no trouble to change the ROIstats interface to be fully backward-compatible instead.

Acknowledgment

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

@codecov-io
Copy link

codecov-io commented Oct 20, 2018

Codecov Report

Merging #2740 into master will decrease coverage by 3.33%.
The diff coverage is 88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2740      +/-   ##
==========================================
- Coverage   67.42%   64.09%   -3.34%     
==========================================
  Files         340      338       -2     
  Lines       43172    43187      +15     
  Branches     5353     5359       +6     
==========================================
- Hits        29110    27679    -1431     
- Misses      13363    14439    +1076     
- Partials      699     1069     +370
Flag Coverage Δ
#smoketests ?
#unittests 64.09% <88%> (-0.71%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/afni/__init__.py 100% <ø> (ø) ⬆️
nipype/interfaces/afni/preprocess.py 82.4% <100%> (+1%) ⬆️
nipype/interfaces/afni/utils.py 81.89% <83.63%> (+0.14%) ⬆️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/freesurfer/base.py 49.59% <0%> (-30.9%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
nipype/algorithms/rapidart.py 35.39% <0%> (-29.21%) ⬇️
nipype/interfaces/spm/base.py 58.41% <0%> (-29.05%) ⬇️
nipype/utils/provenance.py 55.73% <0%> (-28.99%) ⬇️
... and 49 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 eacd567...8c65b81. Read the comment docs.

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.

This looks good. Thanks for the contribution. A few suggestions.

mandatory=True,
exists=True)
mask = File(desc='input mask', argstr='-mask %s', position=3, exists=True)
mask_file = File(desc='input mask', argstr='-mask %s', exists=True)
Copy link
Member

Choose a reason for hiding this comment

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

When renaming a field, you should use deprecated and new_name (see the developer guide) fields (in addition to the new trait).

mask = File(desc='input mask', argstr='-mask %s', position=3, exists=True,
            deprecated='1.1.4', new_name='mask_file')
mask_file = File(desc='input mask', argstr='-mask %s', exists=True)

'zeromedian': '-median',
'zerosigma': '-sigma',
'zeromode': '-mode'
}
Copy link
Member

Choose a reason for hiding this comment

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

We (try to) conform to PEP8. Can you restyle this as follows (4 space indentation, no double spaces, closing brace matching entries)?

_stat_dict = {
    'mean': '-nzmean',
    ...
    }

chi_sq = traits.Bool(
argstr='-chi_sq',
desc='Output the Friedman chi-squared value in addition to the '
'Kendall\'s W.')
Copy link
Member

Choose a reason for hiding this comment

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

Should there be an output that's defined when this is True?

argstr='-chi_sq',
desc='Output the Friedman chi-squared value in addition to the '
'Kendall\'s W.')
mask = traits.File(
Copy link
Member

Choose a reason for hiding this comment

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

mask_file for consistency?

'faces': 7,
'edges': 19,
'vertices': 27,
}
Copy link
Member

Choose a reason for hiding this comment

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

Please reformat this dictionary as above.

output_spec = ReHoOutputSpec

def _list_outputs(self):

Copy link
Member

Choose a reason for hiding this comment

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

No blank line here.

@effigies effigies added this to the 1.1.5 milestone Oct 26, 2018
@effigies effigies modified the milestones: 1.1.5, 1.1.4 Oct 30, 2018
@effigies
Copy link
Member

@rciric Could you update the deprecation to 1.1.4? We can't release until the circle builds are resolved, so might as well get this one in.

@effigies effigies changed the title [ENH] AFNI interfaces: add 3dLocalStat and 3dReHo, rework 3dROIstats [ENH] Add afni.LocalStat and afni.ReHo, update afni.ROIStats inputs Oct 30, 2018
@effigies effigies merged commit 50862c1 into nipy:master Oct 30, 2018
@effigies
Copy link
Member

Thanks!

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.

None yet

3 participants