Skip to content

[ENH] Add interfaces wrapping DIPY worflows #2830

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 23, 2019

Conversation

skoudoro
Copy link
Member

@skoudoro skoudoro commented Dec 14, 2018

Hi Nipype Devs,

The main goal of this PR is to show how to convert a DIPY command line (named workflow) to a Nipype interface. This is a follow up from our discussion with @satra, @arokem, @Garyfallidis and me.

This PR adds :

  • DKI diffusion Model
  • CSA diffusion Model
  • MAPMRI diffusion Model
  • DTI Model
  • Streamline Registration
  • Reslicing
  • Recobundles
  • LabelsBundles
  • Deterministic tracking

TODO:

  • catch and set up default values for the optional arguments and outputs.
  • Add unit Test (I do not know the convention here since they are auto-generated)

@codecov-io
Copy link

codecov-io commented Dec 14, 2018

Codecov Report

Merging #2830 into master will increase coverage by 0.09%.
The diff coverage is 91.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2830      +/-   ##
==========================================
+ Coverage   67.46%   67.55%   +0.09%     
==========================================
  Files         341      342       +1     
  Lines       43347    43622     +275     
  Branches     5377     5452      +75     
==========================================
+ Hits        29243    29470     +227     
- Misses      13407    13454      +47     
- Partials      697      698       +1
Flag Coverage Δ
#smoketests 50.55% <ø> (ø) ⬆️
#unittests 64.97% <91.48%> (+0.11%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/dipy/tracks.py 32.36% <100%> (+3.27%) ⬆️
nipype/interfaces/dipy/registration.py 100% <100%> (ø)
nipype/interfaces/dipy/reconstruction.py 33% <100%> (+3.06%) ⬆️
nipype/interfaces/dipy/base.py 76.57% <87.87%> (+21.02%) ⬆️
nipype/algorithms/confounds.py 69.84% <0%> (+2.58%) ⬆️

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 fbed1f3...ebe49d0. Read the comment docs.

@skoudoro skoudoro changed the title [WIP][NF] DIPY worflows integration [NF] DIPY worflows integration Dec 17, 2018
@skoudoro skoudoro changed the title [NF] DIPY worflows integration [ENH] DIPY worflows integration Dec 17, 2018
@skoudoro
Copy link
Member Author

Hi @satra @effigies,

Any thoughts about this PR ? Any chances to have this PR in for the next release?

@effigies
Copy link
Member

Hi Serge, I think we can shoot for that. I'm not sure how much energy everyone has right now, but we've got a little over a week before we should freeze new merges.

@effigies effigies added this to the 1.1.8 milestone Jan 16, 2019
@skoudoro
Copy link
Member Author

Ok, Thank you for your feedback Chris!

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 LGTM. A couple questions on your log messages.

@effigies
Copy link
Member

@skoudoro Just a quick bump. I think this is basically ready to go if you want to look over my suggestions.

@skoudoro
Copy link
Member Author

skoudoro commented Jan 23, 2019

Thank you for the review and sorry for the late answer, I was travelling.

I applied your suggestion, it was a good catch.

@arokem and I were wondering if it is possible to automate this task for each DIPY release, or during nipype installation. Indeed, It should be easy to catch all workflows from DIPY and then, apply dipy_to_nipype function.

Any suggestions are welcomed!

@arokem
Copy link
Member

arokem commented Jan 23, 2019 via email

@effigies
Copy link
Member

@arokem I agree with that approach. It might also be useful to have a registry, though, that associates interfaces with minimum required dipy versions, so that we can give a useful error when somebody tries to use a more recent workflow with an older dipy installed.

I also agree that that's a problem for another day.

@effigies
Copy link
Member

Oh, and yes, that seems completely plausible.

@effigies effigies changed the title [ENH] DIPY worflows integration [ENH] Add interfaces wrapping DIPY worflows Jan 23, 2019
@effigies effigies merged commit 37e48ec into nipy:master Jan 23, 2019
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
  ...
@skoudoro skoudoro deleted the dipy-worflows-integration branch March 28, 2019 18:47
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.

4 participants