Repository: bids-standard/pybids

Issue:
BIDSFile.copy() does not work on binary files
I am running the following code as a test before I put it in a script. when I run it I get the following error:

```
 x = BIDSLayout("/path/to/bids/directory", derivatives=True) 
 x.get()[2].copy(x.get()[2].filename,False,"/path/to/derivatives/directory","overwright")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/aaron/.local/lib/python3.6/site-packages/bids/layout/models.py", line 268, in copy
    contents = f.read()
  File "/usr/lib/python3.6/codecs.py", line 321, in decode
    (result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x8b in position 1: invalid start byte
```

Is there something wrong with my image files? I can view them just fine on FSL. How would I check?



Discussion:
tyarkoni: I suppose one question is whether we want to keep a `write_contents_to_file()` function and alias it to `write_to_file()` with a deprecation warning. I'm ambivalent, seeing as I doubt anyone's using this directly. But I suppose we might want to retain `BIDSLayout.write_contents_to_file()`, even though I'm skeptical (m)any people are using that too. Thoughts, @effigies?
codecov[bot]: # [Codecov](https://codecov.io/gh/bids-standard/pybids/pull/613?src=pr&el=h1) Report
> Merging [#613](https://codecov.io/gh/bids-standard/pybids/pull/613?src=pr&el=desc) into [master](https://codecov.io/gh/bids-standard/pybids/commit/5fab1dee770babffa7aef668b24cb59a90ba37ef&el=desc) will **increase** coverage by `1.03%`.
> The diff coverage is `87.50%`.

[![Impacted file tree graph](https://codecov.io/gh/bids-standard/pybids/pull/613/graphs/tree.svg?width=650&height=150&src=pr&token=0d39OR1fhx)](https://codecov.io/gh/bids-standard/pybids/pull/613?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
+ Coverage   83.62%   84.66%   +1.03%     
==========================================
  Files          25       25              
  Lines        3194     3378     +184     
  Branches      810      904      +94     
==========================================
+ Hits         2671     2860     +189     
+ Misses        335      330       -5     
  Partials      188      188              
```

| Flag | Coverage Δ | |
|---|---|---|
| #unittests | `84.66% <87.50%> (+1.03%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/bids-standard/pybids/pull/613?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bids/layout/writing.py](https://codecov.io/gh/bids-standard/pybids/pull/613/diff?src=pr&el=tree#diff-Ymlkcy9sYXlvdXQvd3JpdGluZy5weQ==) | `85.29% <75.00%> (-1.31%)` | :arrow_down: |
| [bids/layout/layout.py](https://codecov.io/gh/bids-standard/pybids/pull/613/diff?src=pr&el=tree#diff-Ymlkcy9sYXlvdXQvbGF5b3V0LnB5) | `88.31% <100.00%> (+2.94%)` | :arrow_up: |
| [bids/layout/models.py](https://codecov.io/gh/bids-standard/pybids/pull/613/diff?src=pr&el=tree#diff-Ymlkcy9sYXlvdXQvbW9kZWxzLnB5) | `93.61% <100.00%> (+3.02%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/bids-standard/pybids/pull/613?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/bids-standard/pybids/pull/613?src=pr&el=footer). Last update [5fab1de...01bce70](https://codecov.io/gh/bids-standard/pybids/pull/613?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

effigies: LOL this is in the talk I'm giving tomorrow.

I will warn them that it's deprecated.
tyarkoni: Would you prefer I leave `write_contents_to_file()` alone? I'm okay with that, I just think it's a bit weird to call a method with that name when you want to copy a file. But I do think end users will almost always be using it to write passed contents, and it's mainly `BIDSFile.copy()` that will be internally using it for copying. So I can live with keeping it as-is... just let me know one way or the other.
effigies: I haven't thought about it enough to have an opinion yet. I just added it to demonstrate that PyBIDS has some generative utility, as opposed to just querying.

Patch:
diff --git a/bids/layout/layout.py b/bids/layout/layout.py
index b0d331d0..68b15648 100644
--- a/bids/layout/layout.py
+++ b/bids/layout/layout.py
@@ -19,7 +19,7 @@ from bids_validator import BIDSValidator
 
 from ..utils import listify, natural_sort, make_bidsfile
 from ..external import inflect
-from .writing import build_path, write_contents_to_file
+from .writing import build_path, write_to_file
 from .models import (Base, Config, BIDSFile, Entity, Tag)
 from .index import BIDSLayoutIndexer
 from .utils import BIDSMetadata
@@ -1584,12 +1584,11 @@ class BIDSLayout(object):
             f.copy(path_patterns, symbolic_link=symbolic_links,
                    root=self.root, conflicts=conflicts)
 
-    def write_contents_to_file(self, entities, path_patterns=None,
-                               contents=None, link_to=None,
-                               content_mode='text', conflicts='fail',
-                               strict=False, validate=True):
-        """Write arbitrary data to a file defined by the passed entities and
-        path patterns.
+    def write_to_file(self, entities, path_patterns=None,
+                      contents=None, link_to=None, copy_from=None,
+                      content_mode='text', conflicts='fail',
+                      strict=False, validate=True):
+        """Write data to a file defined by the passed entities and patterns.
 
         Parameters
         ----------
@@ -1632,9 +1631,9 @@ class BIDSLayout(object):
                              "the passed entities given available path "
                              "patterns.")
 
-        write_contents_to_file(path, contents=contents, link_to=link_to,
-                               content_mode=content_mode, conflicts=conflicts,
-                               root=self.root)
+        write_to_file(path, contents=contents, link_to=link_to,
+                      copy_from=copy_from, content_mode=content_mode,
+                      conflicts=conflicts, root=self.root)
 
 
 class Query(enum.Enum):
diff --git a/bids/layout/models.py b/bids/layout/models.py
index cd39426f..e483d45c 100644
--- a/bids/layout/models.py
+++ b/bids/layout/models.py
@@ -12,7 +12,7 @@ import json
 from copy import deepcopy
 from itertools import chain
 
-from .writing import build_path, write_contents_to_file
+from .writing import build_path, write_to_file
 from ..config import get_option
 from .utils import BIDSMetadata
 
@@ -284,17 +284,13 @@ class BIDSFile(Base):
             raise ValueError("Target filename to copy/symlink (%s) doesn't "
                              "exist." % path)
 
+        kwargs = dict(path=new_filename, root=root, conflicts=conflicts)
         if symbolic_link:
-            contents = None
-            link_to = path
+            kwargs['link_to'] = path
         else:
-            with open(path, 'r') as f:
-                contents = f.read()
-            link_to = None
+            kwargs['copy_from'] = path
 
-        write_contents_to_file(new_filename, contents=contents,
-                               link_to=link_to, content_mode='text', root=root,
-                               conflicts=conflicts)
+        write_to_file(**kwargs)
 
 
 class BIDSDataFile(BIDSFile):
diff --git a/bids/layout/writing.py b/bids/layout/writing.py
index 14534a6a..46935c49 100644
--- a/bids/layout/writing.py
+++ b/bids/layout/writing.py
@@ -6,12 +6,13 @@ import warnings
 import os
 import re
 import sys
+import shutil
 from string import Formatter
 from itertools import product
 from ..utils import splitext, listify
 from os.path import join, dirname, exists, islink, isabs, isdir
 
-__all__ = ['build_path', 'write_contents_to_file']
+__all__ = ['build_path', 'write_to_file']
 
 _PATTERN_FIND = re.compile(r'({([\w\d]*?)(?:<([^>]+)>)?(?:\|((?:\.?[\w])+))?\})')
 
@@ -199,11 +200,10 @@ def build_path(entities, path_patterns, strict=False):
     return None
 
 
-def write_contents_to_file(path, contents=None, link_to=None,
-                           content_mode='text', root=None, conflicts='fail'):
+def write_to_file(path, contents=None, link_to=None, copy_from=None,
+                  content_mode='text', root=None, conflicts='fail'):
     """
-    Uses provided filename patterns to write contents to a new path, given
-    a corresponding entity map.
+    Writes provided contents to a new path, or copies from an old path.
 
     Parameters
     ----------
@@ -214,8 +214,11 @@ def write_contents_to_file(path, contents=None, link_to=None,
         to the new path.
     link_to : str
         Optional path with which to create a symbolic link to.
-        Used as an alternative to and takes priority over the contents
+        Used as an alternative to, and takes priority over, the contents
         argument.
+    copy_from : str
+        Optional filename to copy to new location. Used an alternative to, and
+        takes priority over, the contents argument.
     content_mode : {'text', 'binary'}
         Either 'text' or 'binary' to indicate the writing
         mode for the new file. Only relevant if contents is provided.
@@ -267,14 +270,19 @@ def write_contents_to_file(path, contents=None, link_to=None,
     if not exists(dirname(path)):
         os.makedirs(dirname(path))
 
-    if link_to:
+    if link_to is not None:
         os.symlink(link_to, path)
+    elif copy_from is not None:
+        if not exists(copy_from):
+            raise ValueError("Source file '{}' does not exist.".format(copy_from))
+        shutil.copy(copy_from, path)
+
     elif contents:
         mode = 'wb' if content_mode == 'binary' else 'w'
         with open(path, mode) as f:
             f.write(contents)
     else:
-        raise ValueError('One of contents or link_to must be provided.')
+        raise ValueError('One of contents, copy_from or link_to must be provided.')
 
 
 def _expand_options(value):


Test patch:
diff --git a/bids/layout/tests/test_writing.py b/bids/layout/tests/test_writing.py
index 20e4d3fc..476b59df 100644
--- a/bids/layout/tests/test_writing.py
+++ b/bids/layout/tests/test_writing.py
@@ -304,11 +304,11 @@ class TestWritableLayout:
         layout.copy_files(path_patterns=pat, conflicts='overwrite')
         assert exists(example_file)
 
-    def test_write_contents_to_file(self, tmp_bids, layout):
+    def test_write_to_file(self, tmp_bids, layout):
         contents = 'test'
         entities = {'subject': 'Bob', 'session': '01'}
         pat = join('sub-{subject}/ses-{session}/desc.txt')
-        layout.write_contents_to_file(entities, path_patterns=pat,
+        layout.write_to_file(entities, path_patterns=pat,
                                       contents=contents, validate=False)
         target = join(str(tmp_bids), 'bids', 'sub-Bob/ses-01/desc.txt')
         assert exists(target)
@@ -317,12 +317,12 @@ class TestWritableLayout:
         assert written == contents
         assert target not in layout.files
 
-    def test_write_contents_to_file_defaults(self, tmp_bids, layout):
+    def test_write_to_file_defaults(self, tmp_bids, layout):
         contents = 'test'
         entities = {'subject': 'Bob', 'session': '01', 'run': '1',
                     'suffix': 'bold', 'task': 'test', 'acquisition': 'test',
                     'bval': 0}
-        layout.write_contents_to_file(entities, contents=contents)
+        layout.write_to_file(entities, contents=contents)
         target = join(str(tmp_bids), 'bids', 'sub-Bob', 'ses-01',
                       'func', 'sub-Bob_ses-01_task-test_acq-test_run-1_bold.nii.gz')
         assert exists(target)