Coverage Status

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

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

Coverage Status

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

Coverage Status

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

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.

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

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?

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

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 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)

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?

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

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

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