Skip to content

MRG: TOPUP update #794

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 10 commits into from
Apr 2, 2014
Merged

MRG: TOPUP update #794

merged 10 commits into from
Apr 2, 2014

Conversation

satra
Copy link
Member

@satra satra commented Feb 10, 2014

  • allows specifying different phase encode directions for individual volumes
  • simplifies output naming
  • clarifies mandatory requirements
  • update workflows that depend on these interfaces

@satra satra mentioned this pull request Feb 10, 2014
* master:
  fix: moving save from numpy to pickle
  install ordereddict for py26
  fix: moving save from numpy to pickle
  install ordereddict for py26
  fix: update conda in environment
  fix: parenthesis fixes
  dbg: anaconda to miniconda update
  dbg: conda update
  fix: update conda
  ENH: Large SGE jobs were overloading on qstat
@satra
Copy link
Member Author

satra commented Feb 19, 2014

@arnaudbore - could you test this branch out? it seems to be fine with the things i am running.

@arnaudbore
Copy link

Hi Satra, I still have the error message. I'm working with the master I guess. How can I be sure of the version that I'm using ?

Arnaud

@satra
Copy link
Member Author

satra commented Feb 19, 2014

assuming you have no local changes in the nipype repo you can do this:

git remote add satra https://github.com/satra/nipype.git
git fetch satra
git checkout -b fix/eddy satra/fix/eddy

@arnaudbore
Copy link

I'm sorry satra it doesn't work well. I still have the error message Look what I'm doing:

git clone https://github.com/nipy/nipype.git
cd nipype
git remote add satra https://github.com/satra/nipype.git
git fetch satra
git checkout -b fix/eddy satra/fix/eddy
and finally sudo python2.7 setup.py install

I looked into /usr/local/lib/python2.7/dist-packages/nipype and the files that you changed were actually changed so I think I've done it right but I'm not familiar with git so maybe I'm forgetting something...

Thank you for your help.

@oesteban
Copy link
Contributor

Hi there,

I'm able to help with this by now. What is the status of this right now?, referring to this initial guidelines:

allows specifying different phase encode directions for individual volumes
simplifies output naming
clarifies mandatory requirements
todo:
test for encoding file
update workflows that depend on these interfaces

@arnaudbore
Copy link

Hi,

When I run the topup function with a specific output name it raises an error but different from what we had so far so I think satra fixed the output naming. If I use a differect dirrectory than the current one for the output it still raises me an error because it's looking for the output file in the current directory.

Thank you
Arnaud

@satra
Copy link
Member Author

satra commented Mar 16, 2014

@oesteban and @arnaudbore - this request is ready for a good review. since topup takes a long time to run, it would be nice to have a minimal regression test for this. anyone has a low resolution dataset to test this with?

@oesteban
Copy link
Contributor

Yes, we could (with permission of @ecaruyer) include a low-res version of Phantomas / paper (ISMRM14), to test topup and also any other dwi-derived interface/workflow.

I've used it in this paper (ISBI14), so the test is almost written already (that paper is actually the reason why I implemented that scaffold of an interface for topup).

@ecaruyer
Copy link

Hi all,

Sure, the tool is here to be shared! If you could simply somewhere keep a ref to the phantomas project home on github https://github.com/ecaruyer/phantomas or on my website http://www.emmanuelcaruyer.com/phantomas.php

@satra
Copy link
Member Author

satra commented Apr 2, 2014

@oesteban - i'll merge this and open a different pr for regression tests for nipype.

@oesteban
Copy link
Contributor

oesteban commented Apr 2, 2014

Sounds good :)

satra added a commit that referenced this pull request Apr 2, 2014
@satra satra merged commit e4ce216 into nipy:master Apr 2, 2014
@oesteban
Copy link
Contributor

oesteban commented Sep 1, 2014

I wrote an example that uses TOPUP here: https://github.com/oesteban/nipype/blob/enh/NewEPIArtifactCorrections/examples/dmri_preprocessing.py

And it works nicely as regards TOPUP (motion correction is not very good due to FLIRT limitations, though)

oesteban added a commit to oesteban/nipype that referenced this pull request Sep 1, 2014
* TOPUP: regarding nipy#794, nipy#796 I've cleaned up the docstring and implemented
  the three arguments that were still missing. The problem is that TOPUP
  admits one or more values for them, so the implementation of all the
  traits should be updated.

* Eddy: the implementation of @JensNRAD seems to work out, I reviewed
  the docstring. Close nipy#769.

* Added deprecation notices and minor fixes.
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