Repository: bids-standard/pybids (python)

Issue:
`build_path` chooses the first pattern that matches, even when a better match exists
It looks like `BIDSLayout.build_path()` goes with the first path pattern it finds for which all required entities are given. I ran into a situation where this was blocking access to a better pattern later in the list. A minimal example:
```
from bids.layout import BIDSLayout
data=BIDSLayout('/path/to/my/BIDS/data')
entities = {'subject':'001', 
    'suffix':'T1w',
    'extension':'.nii.gz',
    'space':'MNI' }
patterns = ['sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<nii|nii.gz|json>|nii.gz}',
            'sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_space-{space}][_desc-{desc}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<nii|nii.gz|json>|nii.gz}']
print(data.build_path(entities, patterns, validate=False))
print(data.build_path(entities, patterns[1:], validate=False))
```
outputs the following:
```
sub-001/anat/sub-001_T1w.nii.gz
sub-001/anat/sub-001_space-MNI_T1w.nii.gz
```
Including the first pattern causes the 2nd to never be reached, even though it is better: `patterns[1]` makes use of the 'space' entity which is absent in `patterns[0]`.  Users can work around this by giving the exact patterns they want and no others, but it would be nice if pybids tried to find a pattern which matched all given entities, or failing that, as many as possible.


Pull request:
FIX: Refactor of ``build_path`` and inner machinery
Although some issues have been addressed (#516), it seems that the build_path function is still not digesting correctly some patterns.

This PR:
  - [x] reduces the number of lines to implement the functionality (by removing the ``replace_entities`` function, which is now integrated withing ``build_path``)
  - [x] adds a good bunch of tests (and, in particular, the failing examples of #515) to quickly identify regressions.
  - [x] revises some of the ``default_path_patterns`` in the config.
  - [x] enables doctests (to support the next item)
  - [x] adds the writing module to the docs: https://117-62954915-gh.circle-artifacts.com/0/root/project/doc/_build/html/generated/bids.layout.writing.html#module-bids.layout.writing

Patch:
diff --git a/Makefile b/Makefile
index 5f61023c..78028c3e 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
 .PHONY: doc tutorial travis_tests
 
 travis_tests:
-	pytest -n 2 -v --cov bids --cov-config .coveragerc --cov-report xml:cov.xml bids
+	pytest --doctest-modules -n 2 -v --cov bids --cov-config .coveragerc --cov-report xml:cov.xml bids
 
 tutorial:
 	jupyter nbconvert --execute examples/pybids_tutorial.ipynb
diff --git a/bids/layout/config/bids.json b/bids/layout/config/bids.json
index 52a573a8..fd299f06 100644
--- a/bids/layout/config/bids.json
+++ b/bids/layout/config/bids.json
@@ -80,18 +80,18 @@
     ],
 
     "default_path_patterns": [
-        "sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<nii|nii.gz|json>|nii.gz}",
-        "sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_mod-{modality}]_{suffix<defacemask>}.{extension<nii|nii.gz|json>|nii.gz}",
-        "sub-{subject}[/ses-{session}]/func/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_ce-{ceagent}][_dir-{direction}][_rec-{reconstruction}][_run-{run}][_echo-{echo}]_{suffix<bold|cbv|phase|sbref>}.{extension<nii|nii.gz|json>|nii.gz}",
-        "sub-{subject}[/ses-{session}]/dwi/sub-{subject}[_ses-{session}][_acq-{acquisition}]_{suffix<dwi>}.{extension<bval|bvec|json|nii.gz|nii>|nii.gz}",
-        "sub-{subject}[/ses-{session}]/fmap/sub-{subject}[_ses-{session}][_acq-{acquisition}][_dir-{direction}][_run-{run}]_{fmap<phasediff|magnitude[1-2]|phase[1-2]|fieldmap>}.{extension<nii|nii.gz|json>|nii.gz}",
-        "sub-{subject}[/ses-{session}]/fmap/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}]_dir-{direction}[_run-{run}]_{fmap<epi>}.{extension<nii|nii.gz|json>|nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<anat>|anat}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<nii|nii.gz|json>|nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<anat>|anat}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}][_mod-{modality}]_{suffix<defacemask>}.{extension<nii|nii.gz|json>|nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<func>|func}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_ce-{ceagent}][_dir-{direction}][_rec-{reconstruction}][_run-{run}][_echo-{echo}]_{suffix<bold|cbv|phase|sbref>}.{extension<nii|nii.gz|json>|nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<dwi>|dwi}/sub-{subject}[_ses-{session}][_acq-{acquisition}]_{suffix<dwi>}.{extension<bval|bvec|json|nii.gz|nii>|nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<fmap>|fmap}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_dir-{direction}][_run-{run}]_{fmap<phasediff|magnitude[12]|phase[12]|fieldmap>}.{extension<nii|nii.gz|json>|nii.gz}",
+        "sub-{subject}[/ses-{session}]/{datatype<fmap>|fmap}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}]_dir-{direction}[_run-{run}]_{fmap<epi>}.{extension<nii|nii.gz|json>|nii.gz}",
         "sub-{subject}[/ses-{session}]/[{datatype<func|meg|beh>|func}/]sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_rec-{reconstruction}][_run-{run}][_echo-{echo}][_recording-{recording}]_{suffix<events>}.{extension<tsv|json>|tsv}",
         "sub-{subject}[/ses-{session}]/[{datatype<func|beh>|func}/]sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_rec-{reconstruction}][_run-{run}][_echo-{echo}][_recording-{recording}]_{suffix<physio|stim>}.{extension<tsv.gz|json>|tsv.gz}",
-        "sub-{subject}[/ses-{session}]/meg/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_run-{run}][_proc-{proc}]_{suffix<meg>}.{extension}",
-        "sub-{subject}[/ses-{session}]/meg/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_run-{run}][_proc-{proc}]_{suffix<channels>}.{extension<tsv|json>|tsv}",
-        "sub-{subject}[/ses-{session}]/meg/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}]_{suffix<coordsystem>}.{extension<json>|json}",
-        "sub-{subject}[/ses-{session}]/meg/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}]_{suffix<photo>}.{extension<jpg>|jpg}",
+        "sub-{subject}[/ses-{session}]/{datatype<meg>|meg}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_run-{run}][_proc-{proc}]_{suffix<meg>}.{extension}",
+        "sub-{subject}[/ses-{session}]/{datatype<meg>|meg}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_run-{run}][_proc-{proc}]_{suffix<channels>}.{extension<tsv|json>|tsv}",
+        "sub-{subject}[/ses-{session}]/{datatype<meg>|meg}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}]_{suffix<coordsystem>}.{extension<json>|json}",
+        "sub-{subject}[/ses-{session}]/{datatype<meg>|meg}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}]_{suffix<photo>}.{extension<jpg>|jpg}",
         "[acq-{acquisition}_][ce-{ceagent}_][rec-{reconstruction}_]{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<json>|json}",
         "[acq-{acquisition}_][ce-{ceagent}_][rec-{reconstruction}_][mod-{modality}_]{suffix<defacemask>}.{extension<json>|json}",
         "task-{task}[_acq-{acquisition}][_ce-{ceagent}][_dir-{direction}][_rec-{reconstruction}][_run-{run}][_echo-{echo}]_{suffix<bold|cbv|phase|sbref>}.{extension<json>|json}",
diff --git a/bids/layout/layout.py b/bids/layout/layout.py
index 6b255c66..6a2bec56 100644
--- a/bids/layout/layout.py
+++ b/bids/layout/layout.py
@@ -299,30 +299,12 @@ class BIDSLayout(object):
 
     def __repr__(self):
         """Provide a tidy summary of key properties."""
-        n_subjects = len(
-            [s.value
-             for s in self.session.query(Tag).filter_by(
-                 entity_name='subject').group_by(Tag._value)]
-            )
-
-        n_sessions = len(
-            set(
-                (t.value, t.file.entities.get('subject'))
-                 for t in
-                 self.session.query(Tag).filter_by(entity_name='session')
-                 if t.file.entities.get('subject')
-                 )
-            )
-
-        n_runs = len(
-            set(
-                (t.value, t.file.entities.get('subject'))
-                 for t in
-                 self.session.query(Tag).filter_by(entity_name='run')
-                 if t.file.entities.get('subject')
-                 )
-            )
-
+        # TODO: Replace each nested list comprehension with a single DB query
+        n_sessions = len([session for isub in self.get_subjects()
+                          for session in self.get_sessions(subject=isub)])
+        n_runs = len([run for isub in self.get_subjects()
+                      for run in self.get_runs(subject=isub)])
+        n_subjects = len(self.get_subjects())
         root = self.root[-30:]
         s = ("BIDS Layout: ...{} | Subjects: {} | Sessions: {} | "
              "Runs: {}".format(root, n_subjects, n_sessions, n_runs))
@@ -505,7 +487,7 @@ class BIDSLayout(object):
         # We assume something is a BIDS-derivatives dataset if it either has a
         # defined pipeline name, or is applying the 'derivatives' rules.
         pl_name = self.description.get("PipelineDescription", {}).get("Name")
-        is_deriv = bool('derivatives' in self.config)
+        is_deriv = bool(pl_name or ('derivatives' in self.config))
 
         return ((not is_deriv and 'raw' in scope) or
                 (is_deriv and ('derivatives' in scope or pl_name in scope)))
diff --git a/bids/layout/writing.py b/bids/layout/writing.py
index e762ab2a..e3b8b853 100644
--- a/bids/layout/writing.py
+++ b/bids/layout/writing.py
@@ -6,57 +6,14 @@ import warnings
 import os
 import re
 import sys
+from string import Formatter
+from itertools import product
 from ..utils import splitext, listify
 from os.path import join, dirname, exists, islink, isabs, isdir
 
-__all__ = ['replace_entities', 'build_path', 'write_contents_to_file']
+__all__ = ['build_path', 'write_contents_to_file']
 
-
-def replace_entities(entities, pattern):
-    """
-    Replaces all entity names in a given pattern with the corresponding
-    values provided by entities.
-
-    Parameters
-    ----------
-    entities : dict
-        A dictionary mapping entity names to entity values.
-    pattern : str
-        A path pattern that contains entity names denoted
-        by curly braces. Optional portions denoted by square braces.
-        For example: 'sub-{subject}/[var-{name}/]{id}.csv'
-        Accepted entity values, using regex matching, denoted within angle
-        brackets.
-        For example: 'sub-{subject<01|02>}/{task}.csv'
-
-    Returns
-    -------
-    A new string with the entity values inserted where entity names
-    were denoted in the provided pattern.
-    """
-    entities = entities.copy()  # make a local copy, since dicts are mutable
-    ents = re.findall(r'{(.*?)\}', pattern)
-    new_path = pattern
-    for ent in ents:
-        match = re.search(r'([^|<]+)(<.*?>)?(\|.*)?', ent)
-        if match is None:
-            return None
-        name, valid, default = match.groups()
-        default = default[1:] if default is not None else default
-
-        if name in entities and valid is not None:
-            ent_val = str(entities[name])
-            if not re.match(valid[1:-1], ent_val):
-                if default is None:
-                    return None
-                entities[name] = default
-
-        ent_val = entities.get(name, default)
-        if ent_val is None:
-            return None
-        new_path = new_path.replace('{%s}' % ent, str(ent_val))
-
-    return new_path
+_PATTERN_FIND = re.compile(r'({([\w\d]*?)(?:<([^>]+)>)?(?:\|((?:\.?[\w])+))?\})')
 
 
 def build_path(entities, path_patterns, strict=False):
@@ -66,9 +23,13 @@ def build_path(entities, path_patterns, strict=False):
 
     Parameters
     ----------
-    entities : dict
+    entities : :obj:`dict`
         A dictionary mapping entity names to entity values.
-    path_patterns : str or list
+        Entities with ``None`` or empty-string value will be removed.
+        Otherwise, entities will be cast to string values, therefore
+        if any format is expected (e.g., zero-padded integers), the
+        value should be formatted.
+    path_patterns : :obj:`str` or :obj:`list`
         One or more filename patterns to write
         the file to. Entities should be represented by the name
         surrounded by curly braces. Optional portions of the patterns
@@ -78,36 +39,143 @@ def build_path(entities, path_patterns, strict=False):
         the pipe operator. E.g., (e.g., {type<image>|bold} would only match
         the pattern if the entity 'type' was passed and its value is
         "image", otherwise the default value "bold" will be used).
-            Example 1: 'sub-{subject}/[var-{name}/]{id}.csv'
-            Result 2: 'sub-01/var-SES/1045.csv'
-    strict : bool
+    strict : :obj:`bool`
         If True, all passed entities must be matched inside a
         pattern in order to be a valid match. If False, extra entities will
         be ignored so long as all mandatory entities are found.
 
     Returns
     -------
-    A constructed path for this file based on the provided patterns.
+    A constructed path for this file based on the provided patterns, or
+    ``None`` if no path was built given the combination of entities and patterns.
+
+    Examples
+    --------
+    >>> entities = {
+    ...     'extension': 'nii',
+    ...     'space': 'MNI',
+    ...     'subject': '001',
+    ...     'suffix': 'inplaneT2',
+    ... }
+    >>> patterns = ['sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}]'
+    ...             '[_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}]_'
+    ...             '{suffix<T[12]w|T1rho|T[12]map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|'
+    ...             'inplaneT[12]|angio>}.{extension<nii|nii.gz|json>|nii.gz}',
+    ...             'sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}]'
+    ...             '[_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}]'
+    ...             '[_space-{space}][_desc-{desc}]_{suffix<T1w|T2w|T1rho|T1map|T2map|'
+    ...             'T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.'
+    ...             '{extension<nii|nii.gz|json>|nii.gz}']
+    >>> build_path(entities, patterns)
+    'sub-001/anat/sub-001_inplaneT2.nii'
+
+    >>> build_path(entities, patterns, strict=True)
+    'sub-001/anat/sub-001_space-MNI_inplaneT2.nii'
+
+    >>> entities['space'] = None
+    >>> build_path(entities, patterns, strict=True)
+    'sub-001/anat/sub-001_inplaneT2.nii'
+
+    >>> # If some entity is set to None, they are dropped
+    >>> entities['extension'] = None
+    >>> build_path(entities, patterns, strict=True)
+    'sub-001/anat/sub-001_inplaneT2.nii.gz'
+
+    >>> # If some entity is set to empty-string, they are dropped
+    >>> entities['extension'] = ''
+    >>> build_path(entities, patterns, strict=True)
+    'sub-001/anat/sub-001_inplaneT2.nii.gz'
+
+    >>> # If some selector is not in the pattern, skip it...
+    >>> entities['datatype'] = 'anat'
+    >>> build_path(entities, patterns)
+    'sub-001/anat/sub-001_inplaneT2.nii.gz'
+
+    >>> # ... unless the pattern should be strictly matched
+    >>> entities['datatype'] = 'anat'
+    >>> build_path(entities, patterns, strict=True) is None
+    True
+
+    >>> # If the value of an entity is not valid, do not match the pattern
+    >>> entities['suffix'] = 'bold'
+    >>> build_path(entities, patterns) is None
+    True
+
+    >>> entities = {
+    ...     'extension': 'bvec',
+    ...     'subject': '001',
+    ... }
+    >>> patterns = (
+    ...     "sub-{subject}[/ses-{session}]/{datatype|dwi}/sub-{subject}[_ses-{session}]"
+    ...     "[_acq-{acquisition}]_{suffix|dwi}.{extension<bval|bvec|json|nii.gz|nii>|nii.gz}"
+    ... )
+    >>> build_path(entities, patterns, strict=True)
+    'sub-001/dwi/sub-001_dwi.bvec'
+
     """
     path_patterns = listify(path_patterns)
 
+    # One less source of confusion
+    if 'extension' in entities and entities['extension'] is not None:
+        entities['extension'] = entities['extension'].lstrip('.')
+
+    # Drop None and empty-strings, keep zeros
+    entities = {k: v for k, v in entities.items() if v or v == 0}
+
     # Loop over available patherns, return first one that matches all
     for pattern in path_patterns:
+        entities_matched = list(_PATTERN_FIND.findall(pattern))
+        defined = [e[1] for e in entities_matched]
+
         # If strict, all entities must be contained in the pattern
         if strict:
-            defined = re.findall(r'{(.*?)(?:<[^>]+>)?\}', pattern)
             if set(entities.keys()) - set(defined):
                 continue
+
         # Iterate through the provided path patterns
         new_path = pattern
-        optional_patterns = re.findall(r'\[(.*?)\]', pattern)
-        # First build from optional patterns if possible
-        for optional_pattern in optional_patterns:
-            optional_chunk = replace_entities(entities, optional_pattern) or ''
-            new_path = new_path.replace('[%s]' % optional_pattern,
-                                        optional_chunk)
-        # Replace remaining entities
-        new_path = replace_entities(entities, new_path)
+
+        # Expand options within valid values and
+        # check whether entities provided have acceptable value
+        tmp_entities = entities.copy()  # Do not modify the original query
+        for fmt, name, valid, defval in entities_matched:
+            valid_expanded = [v for val in valid.split('|') if val
+                              for v in _expand_options(val)]
+            if valid_expanded and defval and defval not in valid_expanded:
+                warnings.warn(
+                    'Pattern "%s" is inconsistent as it defines an invalid default value.' % fmt
+                )
+            if (
+                valid_expanded
+                and name in entities
+                and entities[name] not in valid_expanded
+            ):
+                continue
+
+            if defval and name not in tmp_entities:
+                tmp_entities[name] = defval
+
+            # At this point, valid & default values are checked & set - simplify pattern
+            new_path = new_path.replace(fmt, '{%s}' % name)
+
+        optional_patterns = re.findall(r'(\[.*?\])', new_path)
+        # Optional patterns with selector are cast to mandatory or removed
+        for op in optional_patterns:
+            for ent_name in {k for k, v in entities.items() if v is not None}:
+                if ('{%s}' % ent_name) in op:
+                    new_path = new_path.replace(op, op[1:-1])
+                    continue
+
+            # Surviving optional patterns are removed
+            new_path = new_path.replace(op, '')
+
+        # Replace entities
+        fields = {pat[1] for pat in Formatter().parse(new_path)
+                  if pat[1] and not pat[1].isdigit()}
+        if fields - set(tmp_entities.keys()):
+            continue
+
+        new_path = new_path.format(**tmp_entities)
 
         if new_path:
             return new_path
@@ -191,3 +259,24 @@ def write_contents_to_file(path, contents=None, link_to=None,
             f.write(contents)
     else:
         raise ValueError('One of contents or link_to must be provided.')
+
+
+def _expand_options(value):
+    """
+    Expand optional substrings of valid entity values.
+
+    Examples
+    --------
+    >>> _expand_options('[Jj]son[12]')
+    ['Json1', 'Json2', 'json1', 'json2']
+
+    >>> _expand_options('json')
+    ['json']
+
+    """
+    expand_patterns = re.findall(r'\[(.*?)\]', value)
+    if not expand_patterns:
+        return [value]
+
+    value = re.sub(r'\[(.*?)\]', '%s', value)
+    return [value % _r for _r in product(*expand_patterns)]
diff --git a/bids/reports/report.py b/bids/reports/report.py
index 9f297276..78f19c9e 100644
--- a/bids/reports/report.py
+++ b/bids/reports/report.py
@@ -50,7 +50,7 @@ class BIDSReport(object):
         self.config = config
 
     def generate(self, **kwargs):
-        """Generate the methods section.
+        r"""Generate the methods section.
 
         Parameters
         ----------
@@ -77,7 +77,12 @@ class BIDSReport(object):
         >>> layout = BIDSLayout(join(get_test_data_path(), 'synthetic'))
         >>> report = BIDSReport(layout)
         >>> counter = report.generate(session='01')
-        >>> counter.most_common()[0][0]
+        Number of patterns detected: 1
+        Remember to double-check everything and to replace <deg> with a degree symbol.
+
+        >>> counter.most_common()[0][0]  # doctest: +ELLIPSIS
+        'For session 01:\n\tMR data were...'
+
         """
         descriptions = []
 
diff --git a/bids/variables/io.py b/bids/variables/io.py
index 8a3ab176..b41ae25a 100644
--- a/bids/variables/io.py
+++ b/bids/variables/io.py
@@ -56,7 +56,7 @@ def load_variables(layout, types=None, levels=None, skip_empty=True,
 
     Examples
     --------
-    >>> load_variables(layout, ['events', 'physio'], subject='01')
+    >>> load_variables(layout, ['events', 'physio'], subject='01')  # doctest: +SKIP
     # returns all variables stored in _events.tsv and _physio.tsv.gz files
     # for runs that belong to subject with id '01'.
     """
diff --git a/doc/api.rst b/doc/api.rst
index a6c63861..4d5c1e01 100644
--- a/doc/api.rst
+++ b/doc/api.rst
@@ -20,6 +20,7 @@ API Reference
    bids.layout.parse_file_entities
    bids.layout.add_config_paths
    bids.layout.index.BIDSLayoutIndexer
+   bids.layout.writing
 
 .. currentmodule:: bids
 


Test patch:
diff --git a/bids/layout/tests/test_path_building.py b/bids/layout/tests/test_path_building.py
index 8457c95b..601c5276 100644
--- a/bids/layout/tests/test_path_building.py
+++ b/bids/layout/tests/test_path_building.py
@@ -22,7 +22,7 @@ def test_invalid_file_construction(layout):
     ents = dict(subject='01', run=1, task='resting-state', suffix='bold')
     with pytest.raises(ValueError):
         layout.build_path(ents)
-    
+
     target = "sub-01/func/sub-01_task-resting-state_run-1_bold.nii.gz"
     assert layout.build_path(ents, validate=False) == target
 
@@ -30,4 +30,4 @@ def test_invalid_file_construction(layout):
 def test_failed_file_construction(layout):
     ents = dict(subject='01', fakekey='foobar')
     with pytest.raises(ValueError):
-        layout.build_path(ents)
+        layout.build_path(ents, strict=True)
diff --git a/bids/layout/tests/test_writing.py b/bids/layout/tests/test_writing.py
index 6883afbc..099be470 100644
--- a/bids/layout/tests/test_writing.py
+++ b/bids/layout/tests/test_writing.py
@@ -3,7 +3,7 @@ import os
 import shutil
 from os.path import join, exists, islink, dirname
 
-from bids.layout.writing import build_path
+from bids.layout.writing import build_path, _PATTERN_FIND
 from bids.tests import get_test_data_path
 from bids import BIDSLayout
 from bids.layout.models import BIDSFile, Entity, Tag, Base
@@ -61,6 +61,42 @@ def layout(tmp_bids):
 
 class TestWritableFile:
 
+    def test_parse_pattern_re(self):
+        """Unit tests on the strict entity pattern finder regex."""
+        assert _PATTERN_FIND.findall('{extension<nii|nii.gz|json>|nii.gz}') == [
+            ('{extension<nii|nii.gz|json>|nii.gz}', 'extension', 'nii|nii.gz|json', 'nii.gz')
+        ]
+        assert _PATTERN_FIND.findall('{extension<json|jsld>|json}') == [
+            ('{extension<json|jsld>|json}', 'extension', 'json|jsld', 'json')
+        ]
+        assert _PATTERN_FIND.findall('{task<func|rest>}/r-{run}.nii.gz') == [
+            ('{task<func|rest>}', 'task', 'func|rest', ''),
+            ('{run}', 'run', '', '')
+        ]
+
+        pattern = """\
+sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{ceagent}][_rec-{reconstruction}]\
+[_space-{space}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.\
+{extension<nii|nii.gz|json>|nii.gz}"""
+        assert sorted(_PATTERN_FIND.findall(pattern)) == [
+            ('{acquisition}', 'acquisition', '', ''),
+            ('{ceagent}', 'ceagent', '', ''),
+            ('{extension<nii|nii.gz|json>|nii.gz}', 'extension', 'nii|nii.gz|json', 'nii.gz'),
+            ('{reconstruction}', 'reconstruction', '', ''),
+            ('{session}', 'session', '', ''),
+            ('{session}', 'session', '', ''),
+            ('{space}', 'space', '', ''),
+            ('{subject}', 'subject', '', ''),
+            ('{subject}', 'subject', '', ''),
+            (
+                '{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|'
+                'PD|PDT2|inplaneT[12]|angio>}',
+                'suffix',
+                'T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio',
+                ''
+            )
+        ]
+
     def test_build_path(self, writable_file):
 
         # Single simple pattern
@@ -112,11 +148,37 @@ class TestWritableFile:
         assert build_path({'run': 3}, pats) == 'ses-A/r-3.nii.gz'
 
         # Pattern with both valid and default values
-        pats = ['ses-{session<A|B|C>|D}/r-{run}.nii.gz']
-        assert build_path({'session': 1, 'run': 3}, pats) == 'ses-D/r-3.nii.gz'
-        pats = ['ses-{session<A|B|C>|D}/r-{run}.nii.gz']
+        pats = ['ses-{session<A|B|C|D>|D}/r-{run}.nii.gz']
+        assert build_path({'run': 3}, pats) == 'ses-D/r-3.nii.gz'
+        pats = ['ses-{session<A|B|C|D>|D}/r-{run}.nii.gz']
         assert build_path({'session': 'B', 'run': 3}, pats) == 'ses-B/r-3.nii.gz'
 
+        # Test extensions with dot and warning is issued
+        pats = ['ses-{session<A|B|C>|D}/r-{run}.{extension}']
+        with pytest.warns(UserWarning) as record:
+            assert build_path({'session': 'B', 'run': 3, 'extension': '.nii'},
+                              pats) == 'ses-B/r-3.nii'
+        assert "defines an invalid default value" in record[0].message.args[0]
+
+        # Test expansion of optional characters
+        pats = ['ses-{session<[ABCD]>|D}/r-{run}.{extension}']
+        assert build_path({'session': 'B', 'run': 3, 'extension': '.nii'},
+                          pats) == 'ses-B/r-3.nii'
+
+        # Test default-only patterns are correctly overriden by setting entity
+        entities = {
+            'subject': '01',
+            'extension': 'bvec',
+            'suffix': 'T1rho',
+        }
+        pats = (
+            "sub-{subject}[/ses-{session}]/{datatype|dwi}/sub-{subject}[_ses-{session}]"
+            "[_acq-{acquisition}]_{suffix|dwi}.{extension<bval|bvec|json|nii.gz|nii>|nii.gz}"
+        )
+        assert build_path(entities, pats) == 'sub-01/dwi/sub-01_T1rho.bvec'
+        assert build_path(entities, pats, strict=True) == 'sub-01/dwi/sub-01_T1rho.bvec'
+
+
     def test_strict_build_path(self):
 
         # Test with strict matching--should fail