Repository: bids-standard/pybids
Pull request #826 (2022-03-29 17:48:28)

RF/FIX: Decompose filter construction for special queries and lists
Currently `Query.None` and `Query.ANY` produce unexpected results, such that `get(phase=["mag", None])` to select images that are either explicitly or implicitly magnitude images always returns an empty list.

This PR refactors by:

* Converting all values into a list
* Creating a clause list
* Extracting special queries (NONE, ANY) and adding the the clause list
* Adding additional clauses if there are additional values to match
* Constructing an aggregate OR clause if needed.

Test included. cc @tsalo

Aiming for a 0.14.1 bugfix release so tools can get this without also getting the less obviously-what-you-want #819.

codecov[bot]: # [Codecov](https://codecov.io/gh/bids-standard/pybids/pull/826?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard) Report
> Merging [#826](https://codecov.io/gh/bids-standard/pybids/pull/826?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard) (511354c) into [maint/0.14.x](https://codecov.io/gh/bids-standard/pybids/commit/f58bdbec5fd703628741536d7ea9ab47b2c1acab?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard) (f58bdbe) will **decrease** coverage by `0.27%`.
> The diff coverage is `92.00%`.

```diff
@@               Coverage Diff                @@
##           maint/0.14.x     #826      +/-   ##
================================================
- Coverage         83.56%   83.28%   -0.28%     
================================================
  Files                32       32              
  Lines              3778     3788      +10     
  Branches            991      994       +3     
================================================
- Hits               3157     3155       -2     
- Misses              417      433      +16     
+ Partials            204      200       -4     
```


| [Impacted Files](https://codecov.io/gh/bids-standard/pybids/pull/826?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard) | Coverage Δ | |
|---|---|---|
| [bids/layout/layout.py](https://codecov.io/gh/bids-standard/pybids/pull/826/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard#diff-Ymlkcy9sYXlvdXQvbGF5b3V0LnB5) | `85.40% <92.00%> (-2.51%)` | :arrow_down: |
| [bids/layout/index.py](https://codecov.io/gh/bids-standard/pybids/pull/826/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard#diff-Ymlkcy9sYXlvdXQvaW5kZXgucHk=) | `84.92% <0.00%> (-0.06%)` | :arrow_down: |
| [bids/layout/models.py](https://codecov.io/gh/bids-standard/pybids/pull/826/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard#diff-Ymlkcy9sYXlvdXQvbW9kZWxzLnB5) | `91.69% <0.00%> (+0.33%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/bids-standard/pybids/pull/826?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/bids-standard/pybids/pull/826?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard). Last update [f58bdbe...511354c](https://codecov.io/gh/bids-standard/pybids/pull/826?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bids-standard).


Patch:
diff --git a/bids/layout/layout.py b/bids/layout/layout.py
index fcae1791..e2c07cda 100644
--- a/bids/layout/layout.py
+++ b/bids/layout/layout.py
@@ -745,29 +745,45 @@ class BIDSLayout(object):
             for name, val in filters.items():
                 tag_alias = aliased(Tag)
 
-                if isinstance(val, (list, tuple)) and len(val) == 1:
-                    val = val[0]
-
+                val_list = list(listify(val)) if val is not None else [None]
+                if not val_list:
+                    continue
+
+                none = any_ = False
+                if None in val_list:
+                    none = True
+                    val_list.remove(None)
+                if Query.NONE in val_list:
+                    none = True
+                    val_list.remove(Query.NONE)
+                if Query.ANY in val_list:
+                    any_ = True
+                    val_list.remove(Query.ANY)
+
+                if none and any_:
+                    # Always true, apply no filter
+                    continue
+
+                # Baseline, use join, start accumulating clauses
                 join_method = query.join
+                val_clauses = []
 
-                if val is None or val == Query.NONE:
+                # NONE and ANY get special treatment
+                if none:
                     join_method = query.outerjoin
-                    val_clause = tag_alias._value.is_(None)
-                elif val == Query.ANY:
-                    val_clause = tag_alias._value.isnot(None)
-                elif regex:
-                    if isinstance(val, (list, tuple)):
-                        val_clause = sa.or_(*[
-                            tag_alias._value.op('REGEXP')(str(v))
-                            for v in val
-                        ])
-                    else:
-                        val_clause = tag_alias._value.op('REGEXP')(str(val))
-                else:
-                    if isinstance(val, (list, tuple)):
-                        val_clause = tag_alias._value.in_(val)
-                    else:
-                        val_clause = tag_alias._value == val
+                    val_clauses.append(tag_alias._value.is_(None))
+                if any_:
+                    val_clauses.append(tag_alias._value.isnot(None))
+
+                # Any remaining values
+                if regex:
+                    val_clauses.extend([tag_alias._value.op('REGEXP')(str(v))
+                                        for v in val_list])
+                elif val_list:
+                    val_clauses.append(tag_alias._value.in_(val_list))
+
+                # Looking for intersection with list of vals, so use OR
+                val_clause = sa.or_(*val_clauses) if len(val_clauses) > 1 else val_clauses[0]
 
                 query = join_method(
                     tag_alias,


Test patch:
diff --git a/bids/layout/tests/test_layout.py b/bids/layout/tests/test_layout.py
index be4ee4ec..c3dd9a23 100644
--- a/bids/layout/tests/test_layout.py
+++ b/bids/layout/tests/test_layout.py
@@ -712,6 +712,19 @@ def test_get_with_invalid_filters(layout_ds005):
     l.get(**filters)
 
 
+def test_get_with_query_constants_in_match_list(layout_ds005):
+    l = layout_ds005
+    get1 = l.get(subject='12', run=1, suffix='bold')
+    get_none = l.get(subject='12', run=None, suffix='bold')
+    get_any = l.get(subject='12', run=Query.ANY, suffix='bold')
+    get1_and_none = l.get(subject='12', run=[None, 1], suffix='bold')
+    get1_and_any = l.get(subject='12', run=[Query.ANY, 1], suffix='bold')
+    get_none_and_any = l.get(subject='12', run=[Query.ANY, Query.NONE], suffix='bold')
+    assert set(get1_and_none) == set(get1) | set(get_none)
+    assert set(get1_and_any) == set(get1) | set(get_any)
+    assert set(get_none_and_any) == set(get_none) | set(get_any)
+
+
 def test_load_layout(layout_synthetic_nodb, db_dir):
     db_path = str(db_dir / 'tmp_db')
     layout_synthetic_nodb.save(db_path)