[REF] Add out_dir argument to filewriting functions

[tsalo]

Closes #494.

Changes proposed in this pull request:

• Add out_dir argument to gscontrol functions.
• Add out_dir argument to io functions.
• Remove unnecessary chdir call.

[jbteves]

@tsalo tests are going to fail because you're missing ts_OC; why are we getting rid of it?

[jbteves]

LGTM

[tsalo]

Okay, the current bug is a weird combination of nibabel.filename_parser.splitext_addext, our choices regarding the test data output directory, and how we call filewrite.

We tend to provide io.filewrite with filenames without extensions, because those extensions are overwritten with nii.gz or nii by the function. io.filewrite uses nibabel.filename_parser.splitext_addext to get the extension for the input filename, even when there's an extra extension like .gz.

When we feed in a filename without periods, that's fine. splitext_addext identifies everything as root (i.e., no extension).

When we feed in a filename with periods in the paths but also an extension, that's also fine. splitext_addext correctly identifies the extension.

However, when we feed in a filename with no extension, but with periods in the paths (as we have in our tests), splitext_addext interprets the period in the path as the separator between the filename and its extension, and that breaks our tests here.

Solutions:

1. Require that inputs to filewrite have extensions.
   • Hard to check programmatically since we'd probably rely on splitext_addext, but at least the documentation and internal calls could be consistent.
2. Add an additional step in io.filewrite with os.path.basename.
   • This pretty much means that splitext_addext is wrong, but we want to handle it internally.
3. Open an issue to nibabel.
   • Might take a while for them to deal with it, assuming they consider it an actual bug.

I'm leaning toward 2. @jbteves @emdupre WDYT?

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@6da1283). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master    #500   +/-   ##
========================================
  Coverage          ?   84.6%           
========================================
  Files             ?      41           
  Lines             ?    2916           
  Branches          ?       0           
========================================
  Hits              ?    2467           
  Misses            ?     449           
  Partials          ?       0

---

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 6da1283...ef6dc22. Read the comment docs.

[jbteves]

My interpretation of this is that this is an undefined case. We should probably remove the periods in test case names, and reserve periods for file extensions only. I think that there's not a way for splitext to know whether it's a path with an extension or a filename missing an extension that happens to have periods without using magic numbers (as in file). I think we should roll with (2) even though I'm not sure that splitext is "wrong" for now, and move towards abolishing periods that don't mean "file extension after here."

[tsalo]

I think that there's not a way for splitext to know whether it's a path with an extension or a filename missing an extension that happens to have periods without using magic numbers (as in file)

It's true that the function can't know that a period in a filename is not an extension delimiter, but it should be able to tell that a period in the file's path isn't. Here's an example:

• /path/to/file.name --> cannot know that .name is part of the filename and not a function
• /path/to.file/name --> should know that .file/name is not an extension, since / is a path delimiter.

I've pushed the basename fix (number 2), and it seems to work.

[jbteves]

@tsalo true, the second case is unambiguous. And that is the case we're running into. Hm. It's probably a lot of dev workarounds but it's worth filing with nibabel.

[jbteves]

This appears to be the best solution for now. Please tag this issue if you open in nibabel, or if you want me to I can raise it there and link this.

[jbteves]

@handwerkerd it turns out I'm the one who approved this, so an additional review by me isn't adequate to merge this.

[jbteves]

@tsalo was there a merge conflict before?

[jbteves]

LGTM

[tsalo]

@jbteves yes to the merge conflict an hour ago and then there was another after merging #408.

[jbteves]

LGTM. Pretty excited for this change, thanks @tsalo !

[jbteves]

@dowdlelt requested your review since you helpfully reviewed #408/#508, and this touches on some of the same bits of code. If you don't have time don't sweat it, I'll find someone else.

[dowdlelt]

Everything here looks straightforward to me - cannot hurt to make out directories more flexible, so I'm for it. I see no problems in these changes.

I am a bit confused by the 'remove stabilize' bit though - is that still relevant here? I don't see it in the code...

[tsalo]

Yeah, I must have dealt with it in another PR. I'll remove it from the summary. Thanks for the reviews!