Coverage increased (+0.001%) to 96.211% when pulling99af2f4 on yarikoptic:enh-remove-before-to_filename into82a261c on nipy:master.

Current coverage is 94.21% (diff: 63.63%)

Merging#466 intomaster will decrease coverage by0.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 byCodecov. Last updatea4724b7...cf564b6

Coverage decreased (-0.003%) to 96.207% when pulling429c932 on yarikoptic:enh-remove-before-to_filename into82a261c on nipy:master.

Coverage decreased (-0.008%) to 96.209% when pullingcf564b6 on yarikoptic:enh-remove-before-to_filename intoa4724b7 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 afterexport 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 oflink_target.nii, but when the env var is set, it gives you the new contents oflink.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, wherebar.nii is some image other thannp.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 ifdata isnp.ones((2, 3, 4)) or not.

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

Now the answer depends on whetherfoo.nii is a symlink AND whetherNIBABEL_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 Brettnotifications@github.com wrote:

Right - I realize it is by design, but consider this code, where
bar.nii is some image other thannp.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 ifdata isnp.ones((2, 3, 4)) or not.

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

Now the answer depends on whetherfoo.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_targetbar

will replace the contents oflink_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 ...