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

Python fsl.findimage() function #1080

Merged
merged 4 commits into from Aug 7, 2017
Merged

Conversation

Lestropie
Copy link
Member

Another script fix necessary to get through my test of #1077.

This helper function should be used to locate output images from FSL commands, due to the fact that not all FSL commands / software versions honour the FSLOUTPUTTYPE environment variable.
@Lestropie Lestropie self-assigned this Aug 3, 2017
Unfortunately only works in the absence of SGE; to fix this issue with SGE would require modifying the file.waitFor() function to accept a list of paths, and repeately check to see if _any_ of those paths exist.
@Lestropie
Copy link
Member Author

Lestropie commented Aug 4, 2017

I've successfully executed all scripts affected by these changes.

I also made a bad boo-boo in #1047 here: Because applytopup is now run separately for each phase-encoding direction, the list of its outputs may only contain one entry, and therefore mrcat can't be run... So this very much needs to go with 3.0_RC2: -rpe_none and -rpe_pair are currently broken. Have I mentioned that I resent dwipreproc? Perhaps it's time to make a matrix of tests (not necessarily in the CI) to make sure dwipreproc handles each possible case as it should; the different possibilities are so intertwined that it's really easy to fix one use case but break something else.

Open to review... Any volunteers?

@jdtournier
Copy link
Member

Not sure I'm the best person to be reviewing this... I'm happy for it to be merged if you're 'happy' with it. As you say, this is the kind to things that requires extensive community testing to really identify all the edge cases. It would be good to have tests for all of these, but that will be a lot of work, and definitely won't make it into tag_3.0_RC2... So I'd merge now and add tests later if/when you get the chance...

@thijsdhollander thijsdhollander self-requested a review August 7, 2017 01:36
Copy link
Contributor

@thijsdhollander thijsdhollander 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, but agree with @jdtournier that we'll need to get "community debugging" for these things. Hence, I'd encourage the merge even more. And of course to fix -rpe_none and -rpe_pair... we can't have those being broken; they're the most frequently used scenarios, I reckon.

@Lestropie Lestropie merged commit 7cb5077 into tag_3.0_RC2 Aug 7, 2017
@Lestropie Lestropie deleted the python_fsl_findimage_function branch August 7, 2017 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants