Repository: IAMconsortium/pyam
Pull request #562 (2021-07-16 15:58:38)

Add `get_data_column()`, refactor filtering by the time domain
# 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

This PR adds a utility function `get_data_column(name)` as short-hand (and more efficient implementation) for `df.data[name]`, because it avoids casting the internal `_data` pd.Series to a pd.DataFrame.

This utility function is then used to make filtering by the time domain more performant.


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

[![Impacted file tree graph](https://codecov.io/gh/IAMconsortium/pyam/pull/562/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/562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium)

```diff
@@          Coverage Diff          @@
##            main    #562   +/-   ##
=====================================
  Coverage   93.6%   93.7%           
=====================================
  Files         50      50           
  Lines       5313    5322    +9     
=====================================
+ Hits        4978    4987    +9     
  Misses       335     335           
```


| [Impacted Files](https://codecov.io/gh/IAMconsortium/pyam/pull/562?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium) | Coverage Δ | |
|---|---|---|
| [pyam/core.py](https://codecov.io/gh/IAMconsortium/pyam/pull/562/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium#diff-cHlhbS9jb3JlLnB5) | `94.3% <100.0%> (+<0.1%)` | :arrow_up: |
| [tests/test\_core.py](https://codecov.io/gh/IAMconsortium/pyam/pull/562/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium#diff-dGVzdHMvdGVzdF9jb3JlLnB5) | `100.0% <100.0%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/IAMconsortium/pyam/pull/562?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/562?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=IAMconsortium). Last update [7d77473...ec1545e](https://codecov.io/gh/IAMconsortium/pyam/pull/562?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).

danielhuppmann: While working on this PR, I noticed that **pyam** also has a getter `df[column]`, which returns the same as the new function `df.get_data_column(column)` (if `column` is a dimension of `data`).

I see three options:
 - mark the pandas-style getter as deprecated because the implicit selection whether you get a column from `data` or `meta` is a bit confusing
 - refactor the getter to use the same speed improvement (avoid casting the internal pd.Series to a DataFrame)
 - use the getter instead of the `get_data_column()`

Any thoughts @gidden or anyone else?
gidden: For reference, the implementation is reproduced below:

```
    def __getitem__(self, key):
        _key_check = [key] if isstr(key) else key
        if set(_key_check).issubset(self.meta.columns):
            return self.meta.__getitem__(key)
        else:
            return self.data.__getitem__(key)
```

In which case if `get_data_column()` is indeed faster than `self.data.__getitem__(key)` and is guaranteed to have the same outcome in all situations, then I would simply replace the last line here.

I would advise against deprecation, as this is a pretty innocuous feature and we have no idea how many users are actually using it. Not to mention, it would be almost impossible to update legacy code...
danielhuppmann: > I would advise against deprecation, as this is a pretty innocuous feature and we have no idea how many users are actually using it. Not to mention, it would be almost impossible to update legacy code...

Don't quite agree that it's so innocuous... If you do `df["region"]`, it's intuitive what you get (the column from `df.data`), if you do `df["category"]`, it's also intuitive (the column from `df.meta`, if it exists). But what do you get if you do `df["model"]`?

But if you think a DeprecationWarning is too strong, how about a FutureWarning - so this will give users probably a year's worth of time to update any code...?
gidden: I would strongly prefer not removing/deprecating. Have we heard anyone who has raised an issue with this? 
danielhuppmann: Ok, let's leave it - wil  simply implement the more efficient approach.
gidden: Great, thank you!
danielhuppmann: Implemented the more efficient approach for the direct getter `IamDataFrame[<column>]` and extended the corresponding unit test for good measure...
gidden: Looks great, thanks!

Patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index c010d63..67aa8f3 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -9,6 +9,7 @@ the attribute `_LONG_IDX` as deprecated. Please use `dimensions` instead.
 
 - [#564](https://github.com/IAMconsortium/pyam/pull/564) Add an example with a secondary axis to the plotting gallery 
 - [#563](https://github.com/IAMconsortium/pyam/pull/563) Enable `colors` keyword argument as list in `plot.pie()` 
+- [#562](https://github.com/IAMconsortium/pyam/pull/562) Add `get_data_column()`, refactor filtering by the time domain
 - [#560](https://github.com/IAMconsortium/pyam/pull/560) Add a feature to `swap_year_for_time()`
 - [#559](https://github.com/IAMconsortium/pyam/pull/559) Add attribute `dimensions`, fix compatibility with pandas v1.3
 - [#557](https://github.com/IAMconsortium/pyam/pull/557) Swap time for year keeping subannual resolution
diff --git a/pyam/core.py b/pyam/core.py
index 2be98e1..3ef1f1b 100755
--- a/pyam/core.py
+++ b/pyam/core.py
@@ -230,7 +230,7 @@ class IamDataFrame(object):
         if set(_key_check).issubset(self.meta.columns):
             return self.meta.__getitem__(key)
         else:
-            return self.data.__getitem__(key)
+            return self.get_data_column(key)
 
     def __len__(self):
         return len(self._data)
@@ -365,10 +365,7 @@ class IamDataFrame(object):
 
         return (
             pd.DataFrame(
-                zip(
-                    self._data.index.get_level_values("variable"),
-                    self._data.index.get_level_values("unit"),
-                ),
+                zip(self.get_data_column("variable"), self.get_data_column("unit")),
                 columns=["variable", "unit"],
             )
             .groupby("variable")
@@ -383,6 +380,22 @@ class IamDataFrame(object):
             return pd.DataFrame([], columns=self.dimensions + ["value"])
         return self._data.reset_index()
 
+    def get_data_column(self, column):
+        """Return a `column` from the timeseries data in long format
+
+        Equivalent to :meth:`IamDataFrame.data[column] <IamDataFrame.data>`.
+
+        Parameters
+        ----------
+        column : str
+            The column name.
+
+        Returns
+        -------
+        pd.Series
+        """
+        return pd.Series(self._data.index.get_level_values(column), name=column)
+
     @property
     def dimensions(self):
         """Return the list of `data` columns (index names & data coordinates)"""
@@ -1721,16 +1734,15 @@ class IamDataFrame(object):
                 cat_idx = self.meta[matches].index
                 keep_col = _make_index(self._data, unique=False).isin(cat_idx)
             elif col == "year":
-                _data = (
-                    self.data[col]
-                    if self.time_col != "time"
-                    else self.data["time"].apply(lambda x: x.year)
-                )
+                if self.time_col == "year":
+                    _data = self.get_data_column(col)
+                else:
+                    _data = self.get_data_column("time").apply(lambda x: x.year)
                 keep_col = years_match(_data, values)
 
             elif col == "month" and self.time_col == "time":
                 keep_col = month_match(
-                    self.data["time"].apply(lambda x: x.month), values
+                    self.get_data_column("time").apply(lambda x: x.month), values
                 )
 
             elif col == "day" and self.time_col == "time":
@@ -1742,17 +1754,19 @@ class IamDataFrame(object):
                     wday = False
 
                 if wday:
-                    days = self.data["time"].apply(lambda x: x.weekday())
+                    days = self.get_data_column("time").apply(lambda x: x.weekday())
                 else:  # ints or list of ints
-                    days = self.data["time"].apply(lambda x: x.day)
+                    days = self.get_data_column("time").apply(lambda x: x.day)
 
                 keep_col = day_match(days, values)
 
             elif col == "hour" and self.time_col == "time":
-                keep_col = hour_match(self.data["time"].apply(lambda x: x.hour), values)
+                keep_col = hour_match(
+                    self.get_data_column("time").apply(lambda x: x.hour), values
+                )
 
             elif col == "time" and self.time_col == "time":
-                keep_col = datetime_match(self.data[col], values)
+                keep_col = datetime_match(self.get_data_column("time"), values)
 
             elif col in self.dimensions:
                 lvl_index, lvl_codes = get_index_levels_codes(self._data, col)


Test patch:
diff --git a/tests/test_core.py b/tests/test_core.py
index fb2d1ae..66ccdf4 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -286,7 +286,9 @@ def test_equals_raises(test_pd_df):
 
 
 def test_get_item(test_df):
-    assert test_df["model"].unique() == ["model_a"]
+    """Assert that getting a column from `data` via the direct getter works"""
+    pdt.assert_series_equal(test_df["model"], test_df.data["model"])
+    pdt.assert_series_equal(test_df["variable"], test_df.data["variable"])
 
 
 def test_index(test_df_year):
@@ -330,6 +332,16 @@ def test_dimensions(test_df):
     assert test_df._LONG_IDX == IAMC_IDX + [test_df.time_col]
 
 
+def test_get_data_column(test_df):
+    """Assert that getting a column from the `data` dataframe works"""
+
+    obs = test_df.get_data_column("model")
+    pdt.assert_series_equal(obs, pd.Series(["model_a"] * 6, name="model"))
+
+    obs = test_df.get_data_column(test_df.time_col)
+    pdt.assert_series_equal(obs, test_df.data[test_df.time_col])
+
+
 def test_filter_empty_df():
     # test for issue seen in #254
     df = IamDataFrame(data=df_empty)