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

BF: correct handling of output paths in dipy_quickbundles. #915

Merged
merged 3 commits into from Feb 15, 2016

Conversation

Projects
None yet
2 participants
@jchoude
Contributor

jchoude commented Feb 12, 2016

Now using a splitext call to correctly get the full path of the output file.

Previously, setting the output_file as something in another directory (by using a path like "../other_dir/output.trk") would still output all files in the current directory, with a broken name.

EDIT: I also checked that the other scripts do not have a similar behavior. They don't.

@arokem

This comment has been minimized.

Member

arokem commented Feb 12, 2016

Any chance to get a test of this, so that it doesn't creep back in?

@jchoude

This comment has been minimized.

Contributor

jchoude commented Feb 12, 2016

Yes, incoming.

@arokem

This comment has been minimized.

Member

arokem commented Feb 12, 2016

🍠

@jchoude

This comment has been minimized.

Contributor

jchoude commented Feb 12, 2016

Just to add, as I posted in an edit to the PR description: I also checked that the other scripts do not have a similar behavior. They don't.

# Need to specify an output directory with a "../" style path
# to trigger old bug.
cmd = ["dipy_quickbundles", tracks_file, '--pkl_file', 'mypickle.pkl',
'--out_file', '../output/tracks300.trk']

This comment has been minimized.

@arokem

arokem Feb 14, 2016

Member

I am not sure that this will work well when testing on a Windows platform (because the path separator is hard coded here). I am running a test of that over here: https://ci.appveyor.com/project/arokem/dipy/build/1.0.649, but it might take a couple of hours to finish running.

@jchoude

This comment has been minimized.

Contributor

jchoude commented Feb 15, 2016

Ok I pushed a new version with better handling for the path separators.

@arokem

This comment has been minimized.

Member

arokem commented Feb 15, 2016

Great. Thanks!

arokem added a commit that referenced this pull request Feb 15, 2016

Merge pull request #915 from jchoude/BF_quickbundle_script_path_split
BF: correct handling of output paths in dipy_quickbundles.

@arokem arokem merged commit 3cd9e00 into nipy:master Feb 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment