Repository: IAMconsortium/pyam
Pull request #267 (2019-09-20 09:21:47)

Region aggregation if region missing
# Please confirm that this PR has done the following:

- [x] Tests Added
- [x] Documentation Added
- [x] Description in RELEASE_NOTES.md Added

## Adding to RELEASE_NOTES.md (remove section after adding to RELEASE_NOTES.md)

Please add a single line in the release notes similar to the following:

```
- (#XX)[http://link-to-pr.com] Added feature which does something
```

# Description of PR

Updates pyam so behaviour of `check_aggregate_region` is sensible even if the region your checking is missing.


znicholls: @danielhuppmann @gidden thoughts would be great here. The new tests currently fail in a cryptic way. The question is, if you call `check_aggregate_region` and the region you're checking isn't there, should an error be raised, a warning or just nothing and `None` is returned (cause a region can't be wrong if it's not there...)?
danielhuppmann: From a quick look, it seems that the command tries to return the object as a pivoted `pd.DataFrame` (via `timeseries()`), which doesn't work if the dataframe is empty.

On the question "how do we expect this to behave", I would follow the behaviour in `aggregate_region()` (log message and empty return): https://github.com/IAMconsortium/pyam/blob/127680a8f412659c440655c4238c33cb45b528da/pyam/core.py#L831
coveralls: 
[![Coverage Status](https://coveralls.io/builds/25852320/badge)](https://coveralls.io/builds/25852320)

Coverage decreased (-0.2%) to 87.86% when pulling **2f335a6e7fc578607c757e1c48818c66caf3400e on znicholls:region-aggregation-bug** into **127680a8f412659c440655c4238c33cb45b528da on IAMconsortium:master**.

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

Coverage decreased (-0.2%) to 87.86% when pulling **2f335a6e7fc578607c757e1c48818c66caf3400e on znicholls:region-aggregation-bug** into **127680a8f412659c440655c4238c33cb45b528da on IAMconsortium:master**.

znicholls: @danielhuppmann this should be good to go

Patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 919b0a2..47cce15 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,7 +1,9 @@
 
 # Next Release
 
+- [#269](https://github.com/IAMconsortium/pyam/pull/269) Ensure append doesn't accidentally swap indexes
 - [#268](https://github.com/IAMconsortium/pyam/pull/268) Update `aggregate_region` so it can find variables below sub-cateogories too
+- [#267](https://github.com/IAMconsortium/pyam/pull/267) Make clear error message if variable-region pair is missing when `check_aggregate_region` is called
 - [#261](https://github.com/IAMconsortium/pyam/pull/261) Add a check that `keep` in `filter()` is a boolean
 - [#254](https://github.com/IAMconsortium/pyam/pull/254) Hotfix for aggregating missing regions and filtering empty dataframes
 - [#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.
diff --git a/pyam/core.py b/pyam/core.py
index 5cbf1ff..006b95c 100644
--- a/pyam/core.py
+++ b/pyam/core.py
@@ -232,8 +232,9 @@ class IamDataFrame(object):
             ret.meta = ret.meta.append(other.meta.loc[diff, :], **sort_kwarg)
 
         # append other.data (verify integrity for no duplicates)
-        _data = ret.data.set_index(ret._LONG_IDX).append(
-            other.data.set_index(other._LONG_IDX), verify_integrity=True)
+        _data = ret.data.set_index(sorted(ret._LONG_IDX)).append(
+            other.data.set_index(sorted(other._LONG_IDX)),
+            verify_integrity=True)
 
         # merge extra columns in `data` and set `LONG_IDX`
         ret.extra_cols += [i for i in other.extra_cols
@@ -888,6 +889,11 @@ class IamDataFrame(object):
 
         # filter and groupby data, use `pd.Series.align` for matching index
         rows = self._apply_filters(region=region, variable=variable)
+        if not rows.any():
+            msg = 'variable `{}` does not exist in region `{}`'
+            logger().info(msg.format(variable, region))
+            return
+
         df_region, df_subregions = (
             _aggregate(self.data[rows], ['region', 'variable'])
             .align(df_subregions)


Test patch:
diff --git a/tests/test_feature_aggregate.py b/tests/test_feature_aggregate.py
index e532c57..506cd40 100644
--- a/tests/test_feature_aggregate.py
+++ b/tests/test_feature_aggregate.py
@@ -1,4 +1,5 @@
 import pytest
+import logging
 
 import numpy as np
 import pandas as pd
@@ -82,6 +83,20 @@ def test_check_aggregate_pass(check_aggregate_df):
     assert obs is None
 
 
+def test_check_internal_consistency_no_world_for_variable(
+    check_aggregate_df, caplog
+):
+    assert check_aggregate_df.check_internal_consistency() is None
+    test_df = check_aggregate_df.filter(
+        variable='Emissions|CH4', region='World', keep=False
+    )
+    caplog.set_level(logging.INFO)
+    test_df.check_internal_consistency()
+    warn_idx = caplog.messages.index("variable `Emissions|CH4` does not exist "
+                                     "in region `World`")
+    assert caplog.records[warn_idx].levelname == "INFO"
+
+
 def test_check_aggregate_fail(meta_df):
     obs = meta_df.check_aggregate('Primary Energy', exclude_on_fail=True)
     assert len(obs.columns) == 2
@@ -287,3 +302,11 @@ def test_aggregate_region_components_handling(check_aggregate_regional_df,
     exp.name = "value"
 
     pd.testing.assert_series_equal(res, exp)
+
+
+def test_check_aggregate_region_no_world(check_aggregate_regional_df, caplog):
+    test_df = check_aggregate_regional_df.filter(region='World', keep=False)
+    test_df.check_aggregate_region('Emissions|N2O', region='World')
+    warn_idx = caplog.messages.index("variable `Emissions|N2O` does not exist "
+                                     "in region `World`")
+    assert caplog.records[warn_idx].levelname == "INFO"
diff --git a/tests/test_feature_append_rename_convert.py b/tests/test_feature_append_rename_convert.py
index ea1ad1f..b34d47c 100644
--- a/tests/test_feature_append_rename_convert.py
+++ b/tests/test_feature_append_rename_convert.py
@@ -91,6 +91,37 @@ def test_append_same_scenario(meta_df):
     npt.assert_array_equal(ts.iloc[2], ts.iloc[3])
 
 
+@pytest.mark.parametrize("shuffle_cols", [True, False])
+def test_append_extra_col(test_df, shuffle_cols):
+    base_data = test_df.data.copy()
+
+    base_data["col_1"] = "hi"
+    base_data["col_2"] = "bye"
+    base_df = IamDataFrame(base_data)
+
+    other_data = base_data[base_data["variable"] == "Primary Energy"].copy()
+    other_data["variable"] = "Primary Energy|Gas"
+    other_df = IamDataFrame(other_data)
+
+    if shuffle_cols:
+        c1_idx = other_df._LONG_IDX.index("col_1")
+        c2_idx = other_df._LONG_IDX.index("col_2")
+        other_df._LONG_IDX[c1_idx] = "col_2"
+        other_df._LONG_IDX[c2_idx] = "col_1"
+
+    res = base_df.append(other_df)
+
+    def check_meta_is(iamdf, meta_col, val):
+        for checker in [iamdf.timeseries().reset_index(), iamdf.data]:
+            meta_vals = checker[meta_col].unique()
+            assert len(meta_vals) == 1, meta_vals
+            assert meta_vals[0] == val, meta_vals
+
+    # ensure meta merged correctly
+    check_meta_is(res, "col_1", "hi")
+    check_meta_is(res, "col_2", "bye")
+
+
 def test_append_duplicates(test_df_year):
     other = copy.deepcopy(test_df_year)
     pytest.raises(ValueError, test_df_year.append, other=other)