Repository: IAMconsortium/pyam

Issue:
`filter()` assumes non-starred `dict` as `keep` kwarg
# Observed (user) error

Just ran into a stupid error where I used `df.filter(dict(foo='bar')` instead of `df.filter(**dict(foo='bar')`, so rather than applying the expected `foo='bar'` rule, the function interprets the dict as the `keep` kwarg.

# Suggested solution

Add a check that `keep` is a boolean, raise error otherwise.

Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/25186762/badge)](https://coveralls.io/builds/25186762)

Coverage decreased (-0.2%) to 87.888% when pulling **2094b11860e490ba0101e3cb0bddaec74949617a on danielhuppmann:fix/filter_keep** into **d5a3a9f46a087e92d28754ba91d9ae48c5db638a on IAMconsortium:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/25186762/badge)](https://coveralls.io/builds/25186762)

Coverage decreased (-0.2%) to 87.888% when pulling **2094b11860e490ba0101e3cb0bddaec74949617a on danielhuppmann:fix/filter_keep** into **d5a3a9f46a087e92d28754ba91d9ae48c5db638a on IAMconsortium:master**.

gidden: Looks good @danielhuppmann. One request: can you add an explicit test for a non-bool'd `keep`? I think the last test is actually doing this, but it would be nice to have something explicit (e.g., `keep='foo'`).
gidden: ok, lgtm! will merge when CI passes.
danielhuppmann: thanks @gidden for the review!

Patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 3cf842e..bbd0b84 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,6 +1,7 @@
 
 # Next Release
 
+- [#261](https://github.com/IAMconsortium/pyam/pull/261) Add a check that `keep` in `filter()` is a boolean
 - [#243](https://github.com/IAMconsortium/pyam/pull/243) Update `pyam.iiasa.Connection` to support all public and private database connections. DEPRECATED: the argument 'iamc15' has been deprecated in favor of names as queryable directly from the REST API.
 - [#241](https://github.com/IAMconsortium/pyam/pull/241) Add `set_meta_from_data` feature
 - [#236](https://github.com/IAMconsortium/pyam/pull/236) Add `swap_time_for_year` method and confirm datetime column is compatible with pyam features
diff --git a/pyam/core.py b/pyam/core.py
index e945530..c563188 100644
--- a/pyam/core.py
+++ b/pyam/core.py
@@ -974,6 +974,10 @@ class IamDataFrame(object):
                ('month', 'hour', 'time')
              - 'regexp=True' disables pseudo-regexp syntax in `pattern_match()`
         """
+        if not isinstance(keep, bool):
+            msg = '`filter(keep={}, ...)` is not valid, must be boolean'
+            raise ValueError(msg.format(keep))
+
         _keep = self._apply_filters(**kwargs)
         _keep = _keep if keep else ~_keep
         ret = copy.deepcopy(self) if not inplace else self


Test patch:
diff --git a/tests/test_core.py b/tests/test_core.py
index 3bc0596..e4df621 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -201,8 +201,15 @@ def test_variable_depth_raises(test_df):
     pytest.raises(ValueError, test_df.filter, level='1/')
 
 
-def test_filter_error(test_df):
-    pytest.raises(ValueError, test_df.filter, foo='foo')
+def test_filter_error_illegal_column(test_df):
+    # filtering by column `foo` is not valid
+    pytest.raises(ValueError, test_df.filter, foo='test')
+
+
+def test_filter_error_keep(test_df):
+    # string or non-starred dict was mis-interpreted as `keep` kwarg, see #253
+    pytest.raises(ValueError, test_df.filter, model='foo', keep=1)
+    pytest.raises(ValueError, test_df.filter, dict(model='foo'))
 
 
 def test_filter_year(test_df):