Repository: bids-standard/pybids

Issue:
get_collections fails when meta-data includes lists
For me this error occurs when:

```
bids_analysis = Analysis(bids_layout, model)
bids_analysis.setup(**entities)
```

`TypeError: unhashable type: 'list'`

Looking at the traceback:

```
collections = self.layout.get_collections(self.level, drop_na=drop_na, **kwargs)
```
seems to be what's failing because in:

```
> /celery_worker/src/pybids/bids/variables/entities.py(161)get_nodes()
    159         sort_cols = [sc for sc in sort_cols if sc in set(rows.columns)]
    160         sort_cols += list(set(rows.columns) - set(sort_cols))
--> 161         rows = rows.sort_values(sort_cols)
    162         inds = rows['node_index'].astype(int)
    163         return [self.nodes[i] for i in inds]
```

In this dataaset, `rows` contains some values that are lists because of structured meta-data.

For example:
```
rows['ImageType'] 
0    [ORIGINAL, PRIMARY, FMRI, NONE, ND, NORM, MOSA]
Name: ImageType, dtype: object
```


I can't really make sense of why that requires sorting (if its not being returned), and if only `node_index` is being accesed. But either way, I think we need to add meta-data with lists to the tests, as that's probably why we didn't catch this. 



Discussion:

Patch:
diff --git a/bids/utils.py b/bids/utils.py
index fe078847..0bb16c44 100644
--- a/bids/utils.py
+++ b/bids/utils.py
@@ -1,6 +1,5 @@
 import re
 import os
-from .external import six
 
 
 def listify(obj):
diff --git a/bids/variables/entities.py b/bids/variables/entities.py
index 34061704..a381705a 100644
--- a/bids/variables/entities.py
+++ b/bids/variables/entities.py
@@ -123,6 +123,19 @@ class NodeIndex(Node):
         return results
 
     def get_nodes(self, level=None, entities=None, strict=False):
+        ''' Retrieves all nodes that match the specified criteria.
+
+        Args:
+            level (str): The level of analysis of nodes to return.
+            entities (dict): Entities to filter on. All nodes must have
+                matching values on all defined keys to be included.
+            strict (bool): If True, an exception will be raised if the entities
+                dict contains any keys that aren't contained in the current
+                index.
+
+        Returns:
+            A list of Node instances.
+        '''
 
         entities = {} if entities is None else entities.copy()
 
@@ -155,20 +168,47 @@ class NodeIndex(Node):
             return []
 
         # Sort and return
-        sort_cols = ['subject', 'session', 'task', 'run']
+        sort_cols = ['subject', 'session', 'task', 'run', 'node_index',
+                     'suffix', 'level', 'datatype']
         sort_cols = [sc for sc in sort_cols if sc in set(rows.columns)]
-        sort_cols += list(set(rows.columns) - set(sort_cols))
         rows = rows.sort_values(sort_cols)
         inds = rows['node_index'].astype(int)
         return [self.nodes[i] for i in inds]
 
+    def create_node(self, level, entities, *args, **kwargs):
+        ''' Creates a new child Node.
+
+        Args:
+            level (str): The level of analysis of the new Node.
+            entities (dict): Dictionary of entities belonging to Node
+            args, kwargs: Optional positional or named arguments to pass onto
+                class-specific initializers. These arguments are only used if
+                a Node that matches the passed entities doesn't already exist,
+                and a new one must be created.
+
+        Returns:
+            A Node instance.
+        '''
+
+        if level == 'run':
+            node = RunNode(entities, *args, **kwargs)
+        else:
+            node = Node(level, entities)
+
+        entities = dict(entities, node_index=len(self.nodes), level=level)
+        self.nodes.append(node)
+        node_row = pd.Series(entities)
+        self.index = self.index.append(node_row, ignore_index=True)
+        return node
+
     def get_or_create_node(self, level, entities, *args, **kwargs):
         ''' Retrieves a child Node based on the specified criteria, creating a
         new Node if necessary.
 
         Args:
-            entities (dict): Dictionary of entities specifying which Node to
-                return.
+            level (str): The level of analysis of the Node.
+            entities (dict): Dictionary of entities to include in newly-created
+                Nodes or filter existing ones.
             args, kwargs: Optional positional or named arguments to pass onto
                 class-specific initializers. These arguments are only used if
                 a Node that matches the passed entities doesn't already exist,
@@ -176,7 +216,6 @@ class NodeIndex(Node):
 
         Returns:
             A Node instance.
-
         '''
 
         result = self.get_nodes(level, entities)
@@ -189,15 +228,4 @@ class NodeIndex(Node):
                                  )
             return result[0]
 
-        # Create Node
-        if level == 'run':
-            node = RunNode(entities, *args, **kwargs)
-        else:
-            node = Node(level, entities)
-
-        entities = dict(entities, node_index=len(self.nodes), level=level)
-        self.nodes.append(node)
-        node_row = pd.Series(entities)
-        self.index = self.index.append(node_row, ignore_index=True)
-
-        return node
+        return self.create_node(level, entities, *args, **kwargs)
diff --git a/bids/variables/io.py b/bids/variables/io.py
index cccddc3c..7fa34b50 100644
--- a/bids/variables/io.py
+++ b/bids/variables/io.py
@@ -1,10 +1,13 @@
+from os.path import join
+import warnings
+import json
+
 import numpy as np
 import pandas as pd
-from os.path import join
+
 from bids.utils import listify
 from .entities import NodeIndex
 from .variables import SparseRunVariable, DenseRunVariable, SimpleVariable
-import warnings
 
 
 BASE_ENTITIES = ['subject', 'session', 'task', 'run']
@@ -165,8 +168,41 @@ def _load_time_variables(layout, dataset=None, columns=None, scan_length=None,
                        "available, or manually specify the scan duration.")
                 raise ValueError(msg)
 
-        run = dataset.get_or_create_node('run', entities, image_file=img_f,
-                                         duration=duration, repetition_time=tr)
+        # We don't want to pass all the image file's entities onto get_node(),
+        # as there can be unhashable nested slice timing values, and this also
+        # slows down querying unnecessarily. Instead, pick out files only based
+        # on the core BIDS entities and any entities explicitly passed as
+        # selectors.
+        # TODO: one downside of this approach is the stripped entities also
+        # won't be returned in the resulting node due to the way things are
+        # implemented. Consider adding a flag to control this.
+        select_on = {k: v for (k, v) in entities.items()
+                     if k in BASE_ENTITIES or k in selectors}
+
+        # If a matching node already exists, return it
+        result = dataset.get_nodes('run', select_on)
+
+        if result:
+            if len(result) > 1:
+                raise ValueError("More than one existing Node matches the "
+                                 "specified entities! You may need to pass "
+                                 "additional selectors to narrow the search.")
+            return result[0]
+
+        # Otherwise create a new node and use that.
+        # We first convert any entity values that are currently collections to
+        # JSON strings to prevent nasty hashing problems downstream. Note that
+        # isinstance() isn't as foolproof as actually trying to hash the
+        # value, but the latter is likely to be slower, and since values are
+        # coming from JSON or filenames, there's no real chance of encountering
+        # anything but a list or dict.
+        entities = {
+            k: (json.dumps(v) if isinstance(v, (list, dict)) else v)
+            for (k, v) in entities.items()
+        }
+
+        run = dataset.create_node('run', entities, image_file=img_f,
+                                  duration=duration, repetition_time=tr)
         run_info = run.get_info()
 
         # Process event files
@@ -395,6 +431,7 @@ def _load_tsv_variables(layout, suffix, dataset=None, columns=None,
 
         level = {'scans': 'session', 'sessions': 'subject',
                  'participants': 'dataset'}[suffix]
+
         node = dataset.get_or_create_node(level, f.entities)
 
         ent_cols = list(set(ALL_ENTITIES) & set(_data.columns))


Test patch:
diff --git a/bids/analysis/tests/test_analysis.py b/bids/analysis/tests/test_analysis.py
index f5a989c9..12568325 100644
--- a/bids/analysis/tests/test_analysis.py
+++ b/bids/analysis/tests/test_analysis.py
@@ -43,19 +43,19 @@ def test_get_design_matrix_arguments(analysis):
                   sampling_rate='highest')
     result = analysis['run'].get_design_matrix(**kwargs)[0]
     assert result.sparse is None
-    assert result.dense.shape == (4800, 9)
+    assert result.dense.shape == (4800, 10)
 
     kwargs = dict(run=1, subject='01', mode='dense', force=True,
                   sampling_rate='TR')
     result = analysis['run'].get_design_matrix(**kwargs)[0]
     assert result.sparse is None
-    assert result.dense.shape == (240, 9)
+    assert result.dense.shape == (240, 10)
 
     kwargs = dict(run=1, subject='01', mode='dense', force=True,
                   sampling_rate=0.5)
     result = analysis['run'].get_design_matrix(**kwargs)[0]
     assert result.sparse is None
-    assert result.dense.shape == (240, 9)
+    assert result.dense.shape == (240, 10)
 
     # format='long' should be ignored for dense output
     kwargs = dict(run=1, subject='01', mode='dense', force=True,
diff --git a/bids/tests/data/ds005/task-mixedgamblestask_bold.json b/bids/tests/data/ds005/task-mixedgamblestask_bold.json
index e1e86665..fb2a2e92 100644
--- a/bids/tests/data/ds005/task-mixedgamblestask_bold.json
+++ b/bids/tests/data/ds005/task-mixedgamblestask_bold.json
@@ -1,4 +1,5 @@
 {
     "RepetitionTime": 2.0,
-    "TaskName": "mixed-gambles task"
-}
\ No newline at end of file
+    "TaskName": "mixed-gambles task",
+    "SliceTiming": [0.0, 0.0571, 0.1143, 0.1714, 0.2286, 0.2857]
+}
diff --git a/bids/variables/tests/test_collections.py b/bids/variables/tests/test_collections.py
index dfe29ca6..57786dc9 100644
--- a/bids/variables/tests/test_collections.py
+++ b/bids/variables/tests/test_collections.py
@@ -72,13 +72,13 @@ def test_run_variable_collection_to_df(run_coll):
 
     # All variables dense, wide format
     df = run_coll.to_df(sparse=False)
-    assert df.shape == (230400, 17)
-    extra_cols = {'TaskName', 'RepetitionTime', 'extension'}
+    assert df.shape == (230400, 18)
+    extra_cols = {'TaskName', 'RepetitionTime', 'extension', 'SliceTiming'}
     assert set(df.columns) == (wide_cols | extra_cols) - {'trial_type'}
 
     # All variables dense, wide format
     df = run_coll.to_df(sparse=False, format='long')
-    assert df.shape == (1612800, 12)
+    assert df.shape == (1612800, 13)
     assert set(df.columns) == (long_cols | extra_cols)