Repository: IAMconsortium/pyam
Pull request #550 (2021-06-24 11:10:49)

Refactor the `rename()` method for performance improvement
# Please confirm that this PR has done the following:

- [x] Tests Added
- [x] Documentation Added
- ~Name of contributors Added to AUTHORS.rst~
- [x] Description in RELEASE_NOTES.md Added

# Description of PR

@byersiiasa asked me about the `rename()` method, which was causing some headache in the AR6 processing workflow. I had a look and realized that it was operating on a full DataFrame rather than the indexed Series (still from before the refactoring last summer).

So I refactored it to use the pyam.index-module functions.

## Performance improvement

Using a 140MB-file from the ongoing ENGAGE work, a multi-dimensional rename (touching some regions and some variables) went from 14 seconds to <4 seconds (and I used `pyam.compare()` to assert that they are identical).

## API change

While working on this, I noticed that the current implementation can be used as an equivalent to the `aggregate()` method by mapping several items to the same new name, e.g., `df.rename(variable={"foo": "new", "bar": "new"}`. Currently, this will implicitly apply a groupby-sum, and only raise an error if a variable "new" already exists.

(This error can be silenced with `check_duplicates=False`, in which case groupby-sum will be applied to have a unique timeseries data index).

The new implementation shows a deprecation warning when renaming to a non-unique index, and it will switched to an error with release 1.0.

(The override `check_duplicates=False` will continue to work.)

codecov[bot]: # [Codecov](https://codecov.io/gh/IAMconsortium/pyam/pull/550?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium) Report
> Merging [#550](https://codecov.io/gh/IAMconsortium/pyam/pull/550?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium) (4c0e474) into [main](https://codecov.io/gh/IAMconsortium/pyam/commit/e7828beae490ed10857c9d8cb5a42705a7cf62f0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium) (e7828be) will **decrease** coverage by `0.0%`.
> The diff coverage is `100.0%`.

[![Impacted file tree graph](https://codecov.io/gh/IAMconsortium/pyam/pull/550/graphs/tree.svg?width=650&height=150&src=pr&token=ThxWlNkonJ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium)](https://codecov.io/gh/IAMconsortium/pyam/pull/550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium)

```diff
@@           Coverage Diff           @@
##            main    #550     +/-   ##
=======================================
- Coverage   93.6%   93.5%   -0.1%     
=======================================
  Files         48      48             
  Lines       5301    5311     +10     
=======================================
+ Hits        4963    4971      +8     
- Misses       338     340      +2     
```


| [Impacted Files](https://codecov.io/gh/IAMconsortium/pyam/pull/550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium) | Coverage Δ | |
|---|---|---|
| [pyam/logging.py](https://codecov.io/gh/IAMconsortium/pyam/pull/550/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium#diff-cHlhbS9sb2dnaW5nLnB5) | `59.3% <ø> (ø)` | |
| [pyam/core.py](https://codecov.io/gh/IAMconsortium/pyam/pull/550/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium#diff-cHlhbS9jb3JlLnB5) | `92.5% <100.0%> (-0.2%)` | :arrow_down: |
| [pyam/index.py](https://codecov.io/gh/IAMconsortium/pyam/pull/550/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium#diff-cHlhbS9pbmRleC5weQ==) | `97.8% <100.0%> (+0.3%)` | :arrow_up: |
| [tests/test\_index.py](https://codecov.io/gh/IAMconsortium/pyam/pull/550/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium#diff-dGVzdHMvdGVzdF9pbmRleC5weQ==) | `100.0% <100.0%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/IAMconsortium/pyam/pull/550?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium).
> **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=IAMconsortium)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/IAMconsortium/pyam/pull/550?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium). Last update [e7828be...4c0e474](https://codecov.io/gh/IAMconsortium/pyam/pull/550?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium). 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=IAMconsortium).

byersiiasa: I have done a bit of testing

smaller files (35 MB xlsx) - 11 regions rename
* v0.11.xxx = ~12s
* this version: = ~6s

Large file ~30 million rows (in long format - 650 MB csv)  - 335 regions rename
* v0.11.xxx = ~53s
* this version: = ~49s

which is a bit strange -...

What it appears is actually the time scales with # of regions being renamed
Large file (old vs new):
* 335 regions: 53 vs 49
* 200 regions: 36 vs 34
* 100 regions: 17 vs 17
* 10 regions: 16.5 vs 16.5


Anyways - I think its a good improvement - thanks @danielhuppmann !

danielhuppmann: Thanks @byersiiasa for doing more elaborate tests! Bit surprised that you don't see more of a performance improvement...

But it does make sense that doing a list comprehension with a large mapping inside is not efficient. So I added two things:
 - use pd.Series.replace instead of the mapping inside a list comprehension
 - do a fast-pass if *all rows* of the timeseries data are touched (working directly on the index codes/levels rather the the index-level values)

In my example, this gives another 10-20% performance improvement (now testing a one-item dictionary, several-items and all-items).

Patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index ba10d97..bd12af8 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,6 +1,7 @@
 # Next Release
 
 - [#551](https://github.com/IAMconsortium/pyam/pull/551) Default to IamDataFrame-time-col on the x-axis of plots
+- [#550](https://github.com/IAMconsortium/pyam/pull/550) Refactor the `rename()` method for performance improvement
 - [#549](https://github.com/IAMconsortium/pyam/pull/549) Make `plotly` an optional dependency
 - [#548](https://github.com/IAMconsortium/pyam/pull/548) Add a `unit_mapping` attribute to show a variable-unit dictionary 
 - [#546](https://github.com/IAMconsortium/pyam/pull/546) Fixed logging for recursive aggregation
diff --git a/pyam/core.py b/pyam/core.py
index 6b03d63..9106ed7 100755
--- a/pyam/core.py
+++ b/pyam/core.py
@@ -73,6 +73,7 @@ from pyam.index import (
     get_index_levels_codes,
     get_keep_col,
     verify_index_integrity,
+    replace_index_values,
 )
 from pyam.logging import deprecation_warning
 
@@ -1032,14 +1033,14 @@ class IamDataFrame(object):
     def rename(
         self, mapping=None, inplace=False, append=False, check_duplicates=True, **kwargs
     ):
-        """Rename and aggregate columns using `groupby().sum()` on values
+        """Rename any index dimension or data coordinate.
 
         When renaming models or scenarios, the uniqueness of the index must be
         maintained, and the function will raise an error otherwise.
 
         Renaming is only applied to any data row that matches for all
         columns given in `mapping`. Renaming can only be applied to the `model`
-        and `scenario` columns, or to other data columns simultaneously.
+        and `scenario` columns, or to other data coordinates simultaneously.
 
         Parameters
         ----------
@@ -1052,15 +1053,20 @@ class IamDataFrame(object):
                                     <current_name_2>: <target_name_2>})
 
             or kwargs as `column_name={<current_name_1>: <target_name_1>, ...}`
-        inplace : bool, default False
-            if True, do operation inplace and return None
-        append : bool, default False
-            append renamed timeseries to `self` and return None;
-            else return new `IamDataFrame`
-        check_duplicates: bool, default True
-            check whether conflict between existing and renamed data exists.
-            If True, raise ValueError; if False, rename and merge
+        inplace : bool, optional
+            Do operation inplace and return None.
+        append : bool, optional
+            Whether to append aggregated timeseries data to this instance
+            (if `inplace=True`) or to a returned new instance (if `inplace=False`).
+        check_duplicates : bool, optional
+            Check whether conflicts exist after renaming of timeseries data coordinates.
+            If True, raise a ValueError; if False, rename and merge
             with :meth:`groupby().sum() <pandas.core.groupby.GroupBy.sum>`.
+
+        Returns
+        -------
+        :class:`IamDataFrame` or **None**
+            Aggregated timeseries data as new object or None if `inplace=True`.
         """
         # combine `mapping` arg and mapping kwargs, ensure no rename conflicts
         mapping = mapping or {}
@@ -1091,14 +1097,11 @@ class IamDataFrame(object):
 
         # renaming is only applied where a filter matches for all given columns
         rows = ret._apply_filters(**filters)
-        idx = ret.meta.index.isin(_make_index(ret.data[rows]))
+        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
-        # TODO reactivate this avoidance of creating a copy
-        _data = ret.data
+        # apply renaming changes (for `data` only on the index)
+        _data_index = ret._data.index
 
-        # apply renaming changes
         for col, _mapping in mapping.items():
             if col in META_IDX:
                 _index = pd.DataFrame(index=ret.meta.index).reset_index()
@@ -1108,26 +1111,38 @@ class IamDataFrame(object):
                 ret.meta.index = _index.set_index(META_IDX).index
             elif col not in data_cols:
                 raise ValueError(f"Renaming by {col} not supported!")
-            _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()))
+            _data_index = replace_index_values(_data_index, col, _mapping, rows)
+
+        # check if duplicates exist in the new timeseries data index
+        duplicate_rows = _data_index.duplicated()
+        has_duplicates = any(duplicate_rows)
+
+        # keeping previous behaviour where naming conflicts within the renamed parts
+        # do not raise an error if there is overlap in the renamed data
+        # TODO: deprecated, always raise an error on duplicates for release >=1.0
+        if has_duplicates and check_duplicates:
+            _full_index = _data_index.to_frame(index=False)
+            _conflict = _full_index[~rows].append(_full_index[rows].drop_duplicates())
+            if any(_conflict.duplicated()):
+                _raise_data_error(
+                    "Conflicting data rows between renamed and not-renamed data, "
+                    "use `aggregate()` or `check_duplicates=False` instead",
+                    _data_index[duplicate_rows].to_frame(index=False),
+                )
 
-        # merge using `groupby().sum()` only if duplicates exist
-        if _data[ret._LONG_IDX].duplicated().any():
-            _data = _data.groupby(ret._LONG_IDX).sum().reset_index()
+            # TODO: raise this as data error for release >=1.0
+            else:
+                deprecation_warning(
+                    "Use `aggregate()` or `check_duplicates=False` instead.",
+                    "Overlapping data rows after renaming. This feature",
+                )
+
+        ret._data.index = _data_index
+        ret._set_attributes()
 
-        # overwrite _data
-        ret._data = format_time_col(_data, ret.time_col).set_index(ret._LONG_IDX).value
+        # merge using `groupby().sum()` only if duplicates exist
+        if has_duplicates:
+            ret._data = ret._data.reset_index().groupby(ret._LONG_IDX).sum().value
 
         if not inplace:
             return ret
diff --git a/pyam/index.py b/pyam/index.py
index 6166fbb..f5110f4 100644
--- a/pyam/index.py
+++ b/pyam/index.py
@@ -1,7 +1,7 @@
 import pandas as pd
 import numpy as np
 
-from .utils import _raise_data_error
+from .utils import _raise_data_error, isstr
 
 
 def get_index_levels(index, level):
@@ -40,18 +40,33 @@ def get_keep_col(codes, matches):
     return np.isin(codes, matches)
 
 
-def replace_index_values(df, level, mapping):
-    """Replace one or several category-values at a specific level"""
+def replace_index_values(df, level, mapping, rows=None):
+    """Replace one or several category-values at a specific level (for specific rows)"""
     index = df if isinstance(df, pd.Index) else df.index
 
     n = index._get_level_number(level)
 
-    # replace the levels
+    # if replacing level values with a filter (by rows)
+    if rows is not None and not all(rows):
+        _levels = pd.Series(index.get_level_values(n))
+        renamed_index = replace_index_values(index[rows], level, mapping)
+        _levels[rows] = list(renamed_index.get_level_values(n))
+        _unique_levels = pd.Index(_levels.unique())
+
+        return append_index_level(
+            index=index.droplevel(n),
+            codes=_unique_levels.get_indexer(_levels),
+            level=_unique_levels,
+            name=level,
+            order=index.names,
+        )
+
+    # else, replace the level values for the entire index dimension
     _levels = index.levels[n].map(lambda l: mapping.get(l, l))
     _unique_levels = _levels.unique()
 
     # if no duplicate levels exist after replace, set new levels and return
-    if len(_levels) == len(_unique_levels):
+    if len(index.levels[n]) == len(_unique_levels):
         return index.set_levels(_levels, n)
 
     # if duplicate levels exist, re-map the codes
@@ -62,9 +77,13 @@ def replace_index_values(df, level, mapping):
 
 def append_index_level(index, codes, level, name, order=False):
     """Append a level to a pd.MultiIndex"""
+    if isstr(level):
+        level = [level]
+        codes = [codes] * len(index.codes[0])
+
     new_index = pd.MultiIndex(
-        codes=index.codes + [[codes] * len(index.codes[0])],
-        levels=index.levels + [[level]],
+        codes=index.codes + [codes],
+        levels=index.levels + [level],
         names=index.names + [name],
     )
     if order:
diff --git a/pyam/logging.py b/pyam/logging.py
index 938f0f9..84ed586 100644
--- a/pyam/logging.py
+++ b/pyam/logging.py
@@ -7,9 +7,9 @@ logger = logging.getLogger(__name__)
 
 @contextmanager
 def adjust_log_level(logger="pyam", level="ERROR"):
+    """Context manager to change log level"""
     if isinstance(logger, str):
         logger = logging.getLogger(logger)
-    """Context manager to change log level"""
     old_level = logger.getEffectiveLevel()
     logger.setLevel(level)
     yield


Test patch:
diff --git a/tests/test_index.py b/tests/test_index.py
index 2984854..6fa2014 100644
--- a/tests/test_index.py
+++ b/tests/test_index.py
@@ -26,16 +26,15 @@ def test_get_index_levels_raises(test_df_index):
         # this test ensures that no transitive replacing occurs
     ],
 )
-def test_replace_index_level(test_pd_df, test_df_index, exp_scen, mapping):
+@pytest.mark.parametrize("rows", (None, [False, True, True]))
+def test_replace_index_level(test_pd_df, test_df_index, exp_scen, mapping, rows):
     """Assert that replace_index_value works as expected"""
-    pd_df = test_pd_df.copy()
-    pd_df["scenario"] = exp_scen
-    exp = pd_df.set_index(IAMC_IDX)
 
-    obs = test_df_index.copy()
-    obs.index = replace_index_values(obs, "scenario", mapping)
+    test_pd_df["scenario"] = exp_scen if rows is None else ["scen_a"] + exp_scen[1:]
+    exp = test_pd_df.set_index(IAMC_IDX)
 
-    pdt.assert_frame_equal(exp, obs)
+    test_df_index.index = replace_index_values(test_df_index, "scenario", mapping, rows)
+    pdt.assert_frame_equal(exp, test_df_index)
 
 
 def test_replace_index_level_raises(test_df_index):