Repository: IAMconsortium/pyam (python)

Issue:
Overly ambitious `groupby().sum()´ in `rename()`
## Description of the problem

We identified a silly mistake in the `IAMC 1.5°C Scenario Data` due to the over-ambitious use of `groupby().sum()` in the `rename()` function implementation. See https://github.com/iiasa/ipcc_sr15_scenario_analysis/issues/2 for details.

There were a number of variables with a typo, and when using `rename()` to correct that, I failed to check whether any variable without the typo also existed. This was the case for two variables, so the values are now double compared to what they should be.

## Proposed solution

I would like to add a kwarg `check_duplicate (default True)` to the function, which raises an error message if any rename mapping leads to a conflict with the existing `model-scenario-region-variable-unit-year` index in the data. A user would need to consciously override the check to merge renamed and existing values as part of the rename. Idea: fail with a bang rather than return unexpected results.

The merging of data renamed to the same `variable` name should obviously always work.

@gidden, @OFR-IIASA, any comments?

Pull request:
Add a validation check to `rename()`
# Please confirm that this PR has done the following:

- [x] Tests Added
- [x] Documentation Added
- [x] Description in RELEASE_NOTES.md Added

# Description of PR

This PR adds a validation check to `df.rename()` to prevent accidentally renaming and aggregating existing and renamed variables.

In terms of the new unit test `test_rename_duplicates()`, the user may not be (actively) aware that a variable `test_3` exists in the `IamDataFrame` and not actually want to aggregate `test_1` and `test_3`. With the new feature, the user will receive an error message as default behaviour and has to actively override the validation step.

closes #182

Patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 392a2bb..cf82791 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,6 +1,7 @@
 
 # Next Release
 
+- [#202](https://github.com/IAMconsortium/pyam/pull/202) Extend the `df.rename()` function with a `check_duplicates (default True)` validation option
 - [#199](https://github.com/IAMconsortium/pyam/pull/199) Initializing an `IamDataFrame` accepts kwargs to fill or create from the data any missing required columns
 - [#197](https://github.com/IAMconsortium/pyam/pull/197) Added a `normalize` function that normalizes all data in a data frame to a specific time period.
 - [#195](https://github.com/IAMconsortium/pyam/pull/195) Fix filtering for `time`, `day` and `hour` to use generic `pattern_match()` (if such a column exists) in 'year'-formmatted IamDataFrames
diff --git a/pyam/core.py b/pyam/core.py
index 634d75c..2bd170b 100644
--- a/pyam/core.py
+++ b/pyam/core.py
@@ -531,7 +531,8 @@ class IamDataFrame(object):
 
             return df
 
-    def rename(self, mapping=None, inplace=False, append=False, **kwargs):
+    def rename(self, mapping=None, inplace=False, append=False,
+               check_duplicates=True, **kwargs):
         """Rename and aggregate column entries using `groupby.sum()` on values.
         When renaming models or scenarios, the uniqueness of the index must be
         maintained, and the function will raise an error otherwise.
@@ -551,6 +552,10 @@ class IamDataFrame(object):
             if True, do operation inplace and return None
         append: bool, default False
             if True, append renamed timeseries to IamDataFrame
+        check_duplicates: bool, default True
+            check whether conflict between existing and renamed data exists.
+            If True, raise ValueError; if False, rename and merge
+            with `groupby().sum()`.
         """
         # combine `mapping` arg and mapping kwargs, ensure no rename conflicts
         mapping = mapping or {}
@@ -560,12 +565,16 @@ class IamDataFrame(object):
             raise ValueError(msg)
         mapping.update(kwargs)
 
+        # determine columns that are not `model` or `scenario`
+        data_cols = set(self._LONG_IDX) - set(META_IDX)
+
         # changing index and data columns can cause model-scenario mismatch
         if any(i in mapping for i in META_IDX)\
-                and any(i in mapping for i in ['region', 'variable', 'unit']):
+                and any(i in mapping for i in data_cols):
             msg = 'Renaming index and data cols simultaneously not supported!'
             raise ValueError(msg)
 
+        # translate rename mapping to `filter()` arguments
         filters = {col: _from.keys() for col, _from in mapping.items()}
 
         # if append is True, downselect and append renamed data
@@ -581,6 +590,9 @@ class IamDataFrame(object):
         rows = ret._apply_filters(filters)
         idx = ret.meta.index.isin(_make_index(ret.data[rows]))
 
+        # if `check_duplicates`, do the rename on a copy until after the check
+        _data = ret.data.copy() if check_duplicates else ret.data
+
         # apply renaming changes
         for col, _mapping in mapping.items():
             if col in META_IDX:
@@ -590,11 +602,23 @@ class IamDataFrame(object):
                     raise ValueError('Renaming to non-unique `{}` index!'
                                      .format(col))
                 ret.meta.index = _index.set_index(META_IDX).index
-            elif col not in ['region', 'variable', 'unit']:
+            elif col not in data_cols:
                 raise ValueError('Renaming by `{}` not supported!'.format(col))
-            ret.data.loc[rows, col] = ret.data.loc[rows, col].replace(_mapping)
+            _data.loc[rows, col] = _data.loc[rows, col].replace(_mapping)
+
+        # check if duplicates exist between the renamed and not-renamed data
+        if check_duplicates:
+            merged = (
+                _data.loc[rows, self._LONG_IDX].drop_duplicates().append(
+                    _data.loc[~rows, self._LONG_IDX].drop_duplicates())
+            )
+            if any(merged.duplicated()):
+                msg = 'Duplicated rows between original and renamed data!\n{}'
+                conflict_rows = merged.loc[merged.duplicated(), self._LONG_IDX]
+                raise ValueError(msg.format(conflict_rows.drop_duplicates()))
 
-        ret.data = ret.data.groupby(ret._LONG_IDX).sum().reset_index()
+        # merge using `groupby().sum()`
+        ret.data = _data.groupby(ret._LONG_IDX).sum().reset_index()
 
         if not inplace:
             return ret
@@ -621,8 +645,8 @@ class IamDataFrame(object):
             return ret
 
     def normalize(self, inplace=False, **kwargs):
-        """Normalize data to a given value. Currently only supports normalizing to a
-        specific time
+        """Normalize data to a given value. Currently only supports normalizing
+        to a specific time.
 
         Parameters
         ----------
diff --git a/pyam/utils.py b/pyam/utils.py
index 7351fcd..755ff24 100644
--- a/pyam/utils.py
+++ b/pyam/utils.py
@@ -132,18 +132,12 @@ def format_data(df, **kwargs):
     if isinstance(df, pd.Series):
         df = df.to_frame()
 
-    # ensure that only either `value` or `variable` custom setting is used
-    _cols = ['value', 'variable']
-    if any([i in kwargs for i in _cols]) and \
-            all([i in kwargs or i in df.columns for i in _cols]):
-        raise ValueError('using both `value` and `variable` is not valid!')
-
-    # if `value` arg is given, melt columns and use column name as `variable`
-    if 'value' in kwargs:
+    # if `value` is given but not `variable`,
+    # melt value columns and use column name as `variable`
+    if 'value' in kwargs and 'variable' not in kwargs:
         value = kwargs.pop('value')
         idx = set(df.columns) & (set(IAMC_IDX) | set(['year', 'time']))
         _df = df.set_index(list(idx))
-        print(_df)
         dfs = []
         for v in value if islistable(value) else [value]:
             if v not in df.columns:
@@ -153,10 +147,11 @@ def format_data(df, **kwargs):
             dfs.append(vdf.reset_index())
         df = pd.concat(dfs).reset_index(drop=True)
 
-    # for other columns, do a rename or concat multiple columns to IAMC-style
+    # otherwise, do a fill-by-value or rename columns or concat to IAMC-style
     for col, value in kwargs.items():
         if col in df:
-            raise ValueError('conflict of kwarg with column in dataframe!')
+            raise ValueError('conflict of kwarg with column `{}` in dataframe!'
+                             .format(col))
 
         if isstr(value) and value in df:
             df.rename(columns={value: col}, inplace=True)


Test patch:
diff --git a/tests/test_cast_to_iamc.py b/tests/test_cast_to_iamc.py
index a53b915..1d6da9a 100644
--- a/tests/test_cast_to_iamc.py
+++ b/tests/test_cast_to_iamc.py
@@ -51,3 +51,13 @@ def test_cast_by_column_concat(meta_df):
     df = IamDataFrame(df, model='model_a', variable=['var_1', 'var_2'])
     assert compare(meta_df, df).empty
     pd.testing.assert_frame_equal(df.data, meta_df.data)
+
+
+def test_cast_with_variable_and_value(meta_df):
+    pe_df = meta_df.filter(variable='Primary Energy')
+    df = pe_df.data.rename(columns={'value': 'lvl'}).drop('variable', axis=1)
+
+    df = IamDataFrame(df, variable='Primary Energy', value='lvl')
+
+    assert compare(pe_df, df).empty
+    pd.testing.assert_frame_equal(df.data, pe_df.data.reset_index(drop=True))
diff --git a/tests/test_feature_append_rename_convert.py b/tests/test_feature_append_rename_convert.py
index 9a7c55d..391e0f2 100644
--- a/tests/test_feature_append_rename_convert.py
+++ b/tests/test_feature_append_rename_convert.py
@@ -6,7 +6,7 @@ import pandas as pd
 from numpy import testing as npt
 
 
-from pyam import IamDataFrame, META_IDX, IAMC_IDX
+from pyam import IamDataFrame, META_IDX, IAMC_IDX, compare
 
 
 RENAME_DF = IamDataFrame(pd.DataFrame([
@@ -181,6 +181,24 @@ def test_rename_append(meta_df):
     pd.testing.assert_frame_equal(obs.meta, exp)
 
 
+def test_rename_duplicates():
+    mapping = {'variable': {'test_1': 'test_3'}}
+    pytest.raises(ValueError, RENAME_DF.rename, **mapping)
+
+    obs = RENAME_DF.rename(check_duplicates=False, **mapping)
+
+    exp = IamDataFrame(pd.DataFrame([
+        ['model', 'scen', 'region_a', 'test_2', 'unit', 2, 6],
+        ['model', 'scen', 'region_a', 'test_3', 'unit', 4, 12],
+        ['model', 'scen', 'region_b', 'test_3', 'unit', 4, 8],
+    ], columns=['model', 'scenario', 'region',
+                'variable', 'unit', 2005, 2010],
+    ))
+
+    assert compare(obs, exp).empty
+    pd.testing.assert_frame_equal(obs.data, exp.data)
+
+
 def test_convert_unit():
     df = IamDataFrame(pd.DataFrame([
         ['model', 'scen', 'SST', 'test_1', 'A', 1, 5],