RF/ENH: remove file prior writing in to_filename

[yarikoptic]

Closes #465
TODOs

• Tests, if folks agree that it could be "the way"

Disclaimer: I am still not sure if that is the most kosher behavior. PR is primarily to test the idea, e.g. if it breaks any tests.

May be such behaviour could be controlled/enabled by some configuration/environment setting (e.g. NIBABEL_REMOVE_BEFORE_WRITE=1) so in those cases when it is clearly necessary we could at least easily enable it.

[coveralls]

Coverage increased (+0.001%) to 96.211% when pulling 99af2f4 on yarikoptic:enh-remove-before-to_filename into 82a261c on nipy:master.

[codecov-io]

Current coverage is 94.21% (diff: 63.63%)

Merging #466 into master will decrease coverage by 0.01%

@@             master       #466   diff @@
==========================================
  Files           160        160          
  Lines         21197      21207    +10   
  Methods           0          0          
  Messages          0          0          
  Branches       2266       2269     +3   
==========================================
+ Hits          19975      19981     +6   
- Misses          802        804     +2   
- Partials        420        422     +2   

Powered by Codecov. Last update a4724b7...cf564b6

[coveralls]

Coverage decreased (-0.003%) to 96.207% when pulling 429c932 on yarikoptic:enh-remove-before-to_filename into 82a261c on nipy:master.

[yarikoptic]

or @matthew-brett do you think it would remain kosher to shift this functionality even deeper into to_file_map? ;)

[matthew-brett]

Yes, to do this properly would mean going down into to_file_map, because it's possible to save images with filenames that way, but then, at the moment, you'd have to go into the implementation for each file type (to_file_map not implemented by default).

[coveralls]

Coverage decreased (-0.008%) to 96.209% when pulling cf564b6 on yarikoptic:enh-remove-before-to_filename into a4724b7 on nipy:master.

[matthew-brett]

This change makes me very nervous.

For example:

ln -s link_target.nii link.nii

Now consider:

python
>>> import numpy as np
>>> import nibabel as nib
>>> nib.save(nib.Nifti1Image(np.ones((2, 3, 4)), None), 'link.nii')
>>> data = img.load('link_target.nii').get_data()

This will give a different result after export NIBABEL_REMOVE_BEFORE_TO_FILENAME=1, or in a git-annex path. I'm worried that this introduces significant global state making it less predictable what any given code fragment is going to do.

[yarikoptic]

This will give a different result after export NIBABEL_REMOVE_BEFORE_TO_FILENAME=1, or in a git-annex path.

if by 'different result' you mean that file no longer would be a symlink, that would be the feature not a bug if explicitly requested (env var) or it was a git annex path... the only possible usecase I see which this would violate in case of git-annex is that may someone would want to rely on the fact that files are under annex and locked (i.e. can't be modified), and does expect script to fail in such a case. So may be it shouldn't have annex specific handling after all, but imho env variable (or if there was a centralized configuration setting), should be ok, since asks for such behaviour explicitly

[matthew-brett]

No, I meant that - when the environment variable is not set, the code above will give you the original contents of link_target.nii, but when the env var is set, it gives you the new contents of link.nii. Or is my logic off?

[yarikoptic]

ah, that way. yeah, sure... somewhat by design ;)

[matthew-brett]

Right - I realize it is by design, but consider this code, where bar.nii is some image other than np.ones((2, 3, 4)):

>>> import numpy as np
>>> import nibabel as nib
>>> nib.save(nib.Nifti1Image(np.ones((2, 3, 4)), None), 'foo.nii')
>>> data = img.load('bar.nii').get_data()

Now I have to work out if data is np.ones((2, 3, 4)) or not.

Before your change, the answer depends only on whether foo.nii is a symlink.

Now the answer depends on whether foo.nii is a symlink AND whether NIBABEL_REMOVE_BEFORE_TO_FILENAME is set.

[yarikoptic]

You are writing/reading two different files. Result depends on either first I've is a symlink or not anyways. I could argue that with proposed modification logic would be more straightforward - writing to one Durant change the other (by design)

On August 6, 2016 9:25:26 PM EDT, Matthew Brett notifications@github.com wrote:

Right - I realize it is by design, but consider this code, where
bar.nii is some image other than np.ones((2, 3, 4)):

>>> import numpy as np
>>> import nibabel as nib
>>> nib.save(nib.Nifti1Image(np.ones((2, 3, 4)), None), 'foo.nii')
>>> data = img.load('bar.nii').get_data()

Now I have to work out if data is np.ones((2, 3, 4)) or not.

Before your change, the answer depends only on whether foo.nii is a
symlink.

Now the answer depends on whether foo.nii is a symlink AND whether
NIBABEL_REMOVE_BEFORE_TO_FILENAME is set.

---

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#466 (comment)

[matthew-brett]

If I understand right, git-annex is the biggest (only?) motivation for this change?

Does git-annex symlink to files in the repository? That seems like it would cause chaos for many operations. As you said already, this:

$ echo "foo" > link_target
$ ln -s link_target link
$ echo "bar" > link
$ cat link_target
bar

will replace the contents of link_target, as for the current behavior of Python and nibabel. Does it really make sense to modify nibabel to do something different from the shell?

[nibotmi]

☔ The latest upstream changes (presumably #479) made this pull request unmergeable. Please resolve the merge conflicts.

[yarikoptic]

Omg, nibotmi buildbot talks to us now? How cute! Or it was a human?

[matthew-brett]

A bot - but maybe a bot that just passed the Turing test ...