REF: define related filetypes once.

[ashgillman]

In response to #1612

Refactors using DRY principal, define related files once.

Additionally, can be patched to extend the related filtypes with:

import nipype
nipype.utils.filemanip.related_filetype_sets.extend([
    ('.avh', '.hv', '.v'),
    ('.avs', '.hs', '.s'),
])

Local tests run, and I have checked the above patching with some of my code using Interfiles.

Potential caveats:

• copyfile is recursive, my base condition is not to recurse if the destination file exists (i.e., has been copied). This could lead to infinite recursion if the copy fails. Suggestions are welcome (check for infinite recursion and throw instructive error? Add a new function argument defining whether to check related files so is only checked at top level?)
• behaviour is not identical to before. i.e., before copyfile on checked for related files for .img or .BRIK, and not if the other file part was used. I think this way is better, but some issues may come from using .mat files?

[codecov-io]

Current coverage is 70.29% (diff: 100%)

Merging #1617 into master will decrease coverage by 0.33%

@@             master      #1617   diff @@
==========================================
  Files          1016        993    -23   
  Lines         50928      49929   -999   
  Methods           0          0          
  Messages          0          0          
  Branches       7238       7777   +539   
==========================================
- Hits          35971      35100   -871   
+ Misses        13894      13390   -504   
- Partials       1063       1439   +376   

Powered by Codecov. Last update 5c9a751...4ab5554

[berleant]

Even DRYer--assuming all extensions start with a period, no need to specify the period here!

[ashgillman]

I take your point, but I think that leaving the periods makes the intent more obvious.

Would you suggest something like:

related_filetype_sets = [tuple('.' + type_ext)
                         for type_ext in (
                             ('hdr', 'img', 'mat'),
                             ('BRIK', 'HEAD'),
                         )]

Or just handling it later (i.e., in a function get_related_filetypes that does what you suggest below)

I am still personally opting for leaving the dots in.

[berleant]

I was imagining handling it later--but I'm also not so much of a stickler as to insist on changing this

[berleant]

I would love to see some tests!

[ashgillman]

Good suggestion on using get_related_filetypes from copyfile.

copyfile seems to be tested quite well (except only using analyze files), and I have added tests for get_related_files

[berleant]

Good point on the infinite recursion--of your suggestions above, I think I prefer the add-extra-parameters option.

[berleant]

Argh, tiny phone big fingers!

[ashgillman]

@shoshber Happens to the best of us. 4ab5554 includes the extra parameter option.

[berleant]

I see that! Looks good to me!

[oesteban]

@effigies It looks like we have a comeback of the copy tests failure here (https://s3.amazonaws.com/archive.travis-ci.org/jobs/159763024/log.txt)

======================================================================
FAIL: nipype.utils.tests.test_filemanip.test_recopy(os.stat_result(st_mode=33204, st_ino=3540, st_dev=2049, st_nlink=1, st_uid=1000, st_gid=1000, st_size=0, st_atime=1473817459, st_mtime=1473817459, st_ctime=1473817459), os.stat_result(st_mode=33204, st_ino=3540, st_dev=2049, st_nlink=1, st_uid=1000, st_gid=1000, st_size=0, st_atime=1473817460, st_mtime=1473817459, st_ctime=1473817459), 'Symlink - OS: posix; Copy: True; Hardlink: False')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/miniconda/lib/python3.5/site-packages/numpy/testing/utils.py", line 315, in assert_equal
    assert_equal(actual[k], desired[k], 'item=%r\n%s' % (k, err_msg), verbose)
  File "/home/travis/miniconda/lib/python3.5/site-packages/numpy/testing/utils.py", line 379, in assert_equal
    raise AssertionError(msg)
AssertionError: 
Items are not equal:
item=7
Symlink - OS: posix; Copy: True; Hardlink: False
 ACTUAL: 1473817459
 DESIRED: 1473817460

======================================================================
FAIL: nipype.utils.tests.test_filemanip.test_recopy(os.stat_result(st_mode=33204, st_ino=3541, st_dev=2049, st_nlink=1, st_uid=1000, st_gid=1000, st_size=0, st_atime=1473817459, st_mtime=1473817459, st_ctime=1473817459), os.stat_result(st_mode=33204, st_ino=3541, st_dev=2049, st_nlink=1, st_uid=1000, st_gid=1000, st_size=0, st_atime=1473817460, st_mtime=1473817459, st_ctime=1473817459), 'Symlink - OS: posix; Copy: True; Hardlink: False')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/miniconda/lib/python3.5/site-packages/numpy/testing/utils.py", line 315, in assert_equal
    assert_equal(actual[k], desired[k], 'item=%r\n%s' % (k, err_msg), verbose)
  File "/home/travis/miniconda/lib/python3.5/site-packages/numpy/testing/utils.py", line 379, in assert_equal
    raise AssertionError(msg)
AssertionError: 
Items are not equal:
item=7
Symlink - OS: posix; Copy: True; Hardlink: False
 ACTUAL: 1473817459
 DESIRED: 1473817460

[effigies]

Oh, I see the problem, now. The atime may be getting updated if the test straddles a clock tick, even though the files aren't being changed. Check out 05d6a05 (cherry-pickable). It adds a function to ignore atime and to wait for clock ticks to make the test deterministic.

That said, it increases the runtime of the test from <1s to ~13s, so you may want to satisfy yourselves that the logic works and then remove the _wait_for_tick() calls.

Edit: Incidentally, it's a different failure from before, but with the same cause.

[berleant]

Instead of increasing test time by 13sec, can we just relax the equality requirement? Perhaps with assert_almost_equal and epsilon 1

[effigies]

I wouldn't want to do assert_almost_equal on the whole thing, because inodes that differ by 1 are different inodes. I think removing atime from the comparison is the more conservative choice there. And that fully solves the issue here.

The clock-tick waits are basically the proof that the problem is solved, and the debugging technique I used to figure out the problem. The only advantage to leaving them in is that they will alert you if this problem crops up again. I'm perfectly comfortable removing them, if that's an unacceptable price.

[berleant]

Ah, I see. I vote to remove _wait_for_tick calls, like you suggest.

[effigies]

Cool. Would you rather I submit a separate PR to nipype, a PR to this branch, or just cherry-pick?

The commits to grab: 05d6a05 9573168

[berleant]

I guess a separate PR? Whatever you like best. I tried making it myself and cherrypicking but couldn't figure it out. I may not have the right permissions.