Skip to content

ENH: Support tckgen -select in MRtrix3 v3+ #2823

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 9 commits into from
Jan 21, 2019

Conversation

jguillon
Copy link
Contributor

Summary

Fixes #2821 .

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

  • Switch tckgen option from -number to -select

Acknowledgment

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

@codecov-io
Copy link

codecov-io commented Dec 13, 2018

Codecov Report

Merging #2823 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2823      +/-   ##
==========================================
- Coverage   67.46%   67.44%   -0.02%     
==========================================
  Files         341      341              
  Lines       43392    43393       +1     
  Branches     5383     5383              
==========================================
- Hits        29276    29268       -8     
- Misses      13419    13427       +8     
- Partials      697      698       +1
Flag Coverage Δ
#smoketests 50.53% <50%> (ø) ⬆️
#unittests 64.85% <66.66%> (-0.04%) ⬇️
Impacted Files Coverage Δ
nipype/pipeline/plugins/legacymultiproc.py 61.5% <ø> (-4.5%) ⬇️
nipype/interfaces/dcm2nii.py 48.13% <ø> (ø) ⬆️
nipype/interfaces/spm/preprocess.py 57.03% <0%> (ø) ⬆️
nipype/interfaces/mrtrix3/tracking.py 89.65% <100%> (+0.18%) ⬆️
nipype/interfaces/spm/__init__.py 100% <100%> (ø) ⬆️

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 ebbd062...beb062c. Read the comment docs.

@effigies
Copy link
Member

I guess the question is whether we should support the 0.3.x series or just 3.0. Changing this will break anybody that uses old MRtrix3 but new Nipype.

Perhaps the better option would be to put a max_ver=0.4 on n_tracks and add a select input with a min_ver=3.

@effigies effigies added this to the 1.1.8 milestone Dec 14, 2018
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.

Ah, sorry. I know I suggested using numeric literals, but it looks like the convention is strings. (I think it'll be coerced to a string anyway, but just for consistency's sake...)

If you could also merge master and make specs again, I think this should be just about ready to go.

@effigies effigies changed the title Fix/mrtrix3 tckgen option ENH: Support tckgen -select in MRtrix3 v3+ Dec 22, 2018
@effigies
Copy link
Member

@jguillon Could you merge current master. I think that should fix the tests.

@effigies effigies merged commit f2a21fd into nipy:master Jan 21, 2019
@jguillon jguillon deleted the fix/mrtrix3-tckgen-option branch January 21, 2019 10:27
yarikoptic added a commit to yarikoptic/nipype that referenced this pull request Feb 4, 2019
1.1.8 (January 28, 2019)

  * FIX: ANTS LaplacianThickness cmdline opts fixed up (nipy#2846)
  * FIX: Resolve LinAlgError during SVD (nipy#2838)
  * ENH: Add interfaces wrapping DIPY worflows (nipy#2830)
  * ENH: Update BIDSDataGrabber for pybids 0.7 (nipy#2737)
  * ENH: Add FSL `eddy_quad` interface (nipy#2825)
  * ENH: Support tckgen -select in MRtrix3 v3+ (nipy#2823)
  * ENH: Support for BIDS event files (nipy#2845)
  * ENH: CompositeTransformUtil, new ANTs interface (nipy#2785)
  * RF: Move pytest and pytest-xdist from general requirement into tests_required (nipy#2850)
  * DOC: Add S3DataGrabber example (nipy#2849)
  * DOC: Skip conftest module in API generation (nipy#2852)
  * DOC: Hyperlink DOIs to preferred resolver (nipy#2833)
  * MAINT: Install numpy!=1.16.0 from conda in Docker (nipy#2862)
  * MAINT: Drop pytest-xdist requirement, minimum pytest version  (nipy#2856)
  * MAINT: Disable numpy 1.16.0 for Py2.7 (nipy#2855)

* tag '1.1.8': (79 commits)
  MNT: Add @feilong to .zenodo, update ordering
  MNT: Update .mailmap
  MNT: Update .zenodo ordering
  Accept invitation as Zenodo release co-author (see nipy#2864)
  MAINT: Update .mailmap
  BF: allowing bids_event_file as alternate input
  MNT: Update .zenodo ordering
  MNT: Version 1.1.8
  DOC: 1.1.8 changelog
  Update nipype/interfaces/dipy/tracks.py
  Update nipype/interfaces/dipy/reconstruction.py
  MNT: Install numpy!=1.16.0 from conda in Docker
  Add FSL auto test
  remake specs
  Update nipype/interfaces/io.py
  Remove return type named tuple
  Update nipype/info.py
  STY: Whitespace, line length
  Remove out_ prefix from EddyQuad outputs
  Apply minor edits from code review
  ...
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.

Renamed MRtrix 3.0 tckgen option(s)
3 participants