Repository: mie-lab/trackintel
Pull request #533 (2023-09-20 12:28:20)

ENH: combine `TrackintelGeoDataFrame` and `TrackintelGeoDataFrameWithFallback` into one class
Instead of having two GeoDataFrame subclasses I combined them together into one with some conditionals that check if the `fallback_class` is set. This will hopefully make the code a bit better to read.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/mie-lab/trackintel/pull/533?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab) Report
Patch coverage: **`100.00%`** and project coverage change: **`-0.09%`** :warning:
> Comparison is base [(`fcb163b`)](https://app.codecov.io/gh/mie-lab/trackintel/commit/fcb163bdd94ee8c2072e9af0228462cfaa482a76?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab) 93.45% compared to head [(`6fb9b20`)](https://app.codecov.io/gh/mie-lab/trackintel/pull/533?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab) 93.37%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
- Coverage   93.45%   93.37%   -0.09%     
==========================================
  Files          34       34              
  Lines        2170     2143      -27     
  Branches      416      410       -6     
==========================================
- Hits         2028     2001      -27     
  Misses        129      129              
  Partials       13       13              
```


| [Files Changed](https://app.codecov.io/gh/mie-lab/trackintel/pull/533?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab) | Coverage Δ | |
|---|---|---|
| [trackintel/model/trips.py](https://app.codecov.io/gh/mie-lab/trackintel/pull/533?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9tb2RlbC90cmlwcy5weQ==) | `100.00% <100.00%> (ø)` | |
| [trackintel/model/util.py](https://app.codecov.io/gh/mie-lab/trackintel/pull/533?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9tb2RlbC91dGlsLnB5) | `100.00% <100.00%> (ø)` | |


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/mie-lab/trackintel/pull/533?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab).

bifbof: The changes look really big, but in reality I changed 6 lines and then deleted the unused code. :)
bifbof: Yes, the output of all generating functions is still a TODO with #535
hongyeehh: I could not see new updates. Did you push the commits to this PR? @bifbof 
bifbof: > I could not see new updates. Did you push the commits to this PR? @bifbof

I thought I did. But apparently I forgot it.

Patch:
diff --git a/trackintel/model/trips.py b/trackintel/model/trips.py
index 1a6b98b..fde9d7b 100644
--- a/trackintel/model/trips.py
+++ b/trackintel/model/trips.py
@@ -10,7 +10,7 @@ from trackintel.model.util import (
     _register_trackintel_accessor,
     TrackintelBase,
     TrackintelDataFrame,
-    TrackintelGeoDataFrameWithFallback,
+    TrackintelGeoDataFrame,
 )
 
 
@@ -159,8 +159,8 @@ class TripsDataFrame(TrackintelBase, TrackintelDataFrame):
         return ti.preprocessing.trips.generate_tours(trips=self, **kwargs)
 
 
-# added GeoDataFrame and DataFrame manually afterwards such that our methods always come first
-class TripsGeoDataFrame(TrackintelGeoDataFrameWithFallback, TripsDataFrame, gpd.GeoDataFrame):
+# added GeoDataFrame manually afterwards such that our methods always come first
+class TripsGeoDataFrame(TrackintelGeoDataFrame, TripsDataFrame, gpd.GeoDataFrame):
     """Class to treat a GeoDataFrame as collections of trips.
 
     Requires at least the following columns:
diff --git a/trackintel/model/util.py b/trackintel/model/util.py
index 296c95a..767ee53 100644
--- a/trackintel/model/util.py
+++ b/trackintel/model/util.py
@@ -112,58 +112,6 @@ def _copy_docstring(wrapped, assigned=("__doc__",), updated=[]):
 
 
 def _wrapped_gdf_method(func):
-    """Decorator function that downcast types to trackintel class if is GeoDataFrame and has the required columns."""
-
-    @wraps(func)  # copy all metadata
-    def wrapper(self, *args, **kwargs):
-        result = func(self, *args, **kwargs)
-        if not isinstance(result, GeoDataFrame) or not self._check(result, validate_geometry=False):
-            return result
-        # as geopandas only change the __class__ attribute, we can just change it back
-        result.__class__ = self.__class__
-        return result
-
-    return wrapper
-
-
-class TrackintelGeoDataFrame(GeoDataFrame):
-    """Helper class to subtype GeoDataFrame correctly."""
-
-    @staticmethod
-    def _check(self, validate_geometry=True):
-        raise NotImplementedError
-
-    # Following methods manually set self.__class__ fix to GeoDataFrame.
-    # Thus to properly subtype, we need to downcast them with the _wrapped_gdf_method decorator.
-    @_wrapped_gdf_method
-    def __getitem__(self, key):
-        return super().__getitem__(key)
-
-    @_wrapped_gdf_method
-    def copy(self, deep=True):
-        return super().copy(deep=deep)
-
-    @_wrapped_gdf_method
-    def merge(self, *args, **kwargs):
-        return super().merge(*args, **kwargs)
-
-    @property
-    def _constructor(self):
-        """Interface to subtype pandas properly"""
-        # we loose access to self in inner function -> write objects to local variables
-        super_cons = super()._constructor
-
-        def _constructor_with_fallback(*args, **kwargs):
-            result = super_cons(*args, **kwargs)
-            # cannot validate_geometry as geometry column is maybe not set
-            if isinstance(result, GeoDataFrame) and self._check(result, validate_geometry=False):
-                return self.__class__(result, validate_geometry=False)
-            return result
-
-        return _constructor_with_fallback
-
-
-def _wrapped_gdf_method_fallback(func):
     """Decorator function that downcast types to trackintel class if is (Geo)DataFrame and has the required columns."""
 
     @wraps(func)  # copy all metadata
@@ -172,23 +120,24 @@ def _wrapped_gdf_method_fallback(func):
         if isinstance(result, pd.DataFrame) and self._check(result, validate_geometry=False):
             if isinstance(result, GeoDataFrame):
                 result.__class__ = self.__class__
-            else:
+            elif self.fallback_class is not None:
                 result.__class__ = self.fallback_class
         return result
 
     return wrapper
 
 
-# short and memorizable name
-class TrackintelGeoDataFrameWithFallback(GeoDataFrame):
+class TrackintelGeoDataFrame(GeoDataFrame):
     """
     Helper class to subtype GeoDataFrame correctly and fallback to custom class.
 
-    Fallback to fallback_class if looses geometry but _check does still succeed.
-    Fallback to GeoDataFrame if still has geometry but _check fails.
-    Fallback to DataFrame if looses geometry and _check fails.
+    Three possible outcomes for the _constructor
+    - If check fails or is DataFrame without fallback_class set, return GeoDataFrame/DataFrame instance w/o changes
+    - If check succeeds and has geometry, change class to current class and return
+    - If check succeeds and has no geometry, change class to fallback_class and return
     """
 
+    # set fallback_class for succeeding _check and missing geometry, if None default to pd.DataFrame
     fallback_class = None
 
     @staticmethod
@@ -208,21 +157,23 @@ class TrackintelGeoDataFrameWithFallback(GeoDataFrame):
             if isinstance(result, GeoDataFrame):
                 # cannot validate geometry as geometry column is maybe not set
                 return self.__class__(result, validate_geometry=False)
-            return self.fallback_class(result)
+            if self.fallback_class is not None:
+                return self.fallback_class(result)
+            return result
 
         return _constructor_with_fallback
 
     # Following methods manually set self.__class__ fix to GeoDataFrame.
-    # Thus to properly subtype, we need to downcast them with the _wrapped_gdf_method_fallback decorator.
-    @_wrapped_gdf_method_fallback
+    # Thus to properly subtype, we need to downcast them with the _wrapped_gdf_method decorator.
+    @_wrapped_gdf_method
     def __getitem__(self, key):
         return super().__getitem__(key)
 
-    @_wrapped_gdf_method_fallback
+    @_wrapped_gdf_method
     def copy(self, deep=True):
         return super().copy(deep=deep)
 
-    @_wrapped_gdf_method_fallback
+    @_wrapped_gdf_method
     def merge(self, *args, **kwargs):
         return super().merge(*args, **kwargs)
 


Test patch:
diff --git a/tests/model/test_util.py b/tests/model/test_util.py
index 1c84dcd..8a06fb2 100644
--- a/tests/model/test_util.py
+++ b/tests/model/test_util.py
@@ -16,10 +16,8 @@ from trackintel.model.util import (
     _copy_docstring,
     _register_trackintel_accessor,
     _wrapped_gdf_method,
-    _wrapped_gdf_method_fallback,
     get_speed_positionfixes,
     TrackintelGeoDataFrame,
-    TrackintelGeoDataFrameWithFallback,
     TrackintelDataFrame,
 )
 
@@ -243,19 +241,23 @@ class Test_copy_docstring:
 
 
 class Test_wrapped_gdf_method:
-    def test_no_geodataframe(self, example_positionfixes):
-        """Test if function return value does not subclass GeoDataFrame then __class__ is not touched"""
+    """Test if _wrapped_gdf_method conditionals work"""
+
+    def test_no_dataframe(self, example_positionfixes):
+        """Test if function return value does not subclass DataFrame then __class__ is not touched"""
 
         @_wrapped_gdf_method
-        def foo(gdf: GeoDataFrame) -> pd.DataFrame:
-            return gdf.drop(columns=gdf.geometry.name)
+        def foo(gdf: GeoDataFrame) -> pd.Series:
+            return pd.Series(gdf.iloc[0])
 
-        assert type(foo(example_positionfixes)) is pd.DataFrame
+        assert type(foo(example_positionfixes)) is pd.Series
 
     def test_failed_check(self, example_positionfixes):
         """Test if _check fails then __class__ is not touched"""
 
         class A(GeoDataFrame):
+            fallback_class = None
+
             @staticmethod
             def _check(obj, validate_geometry=True):
                 return False
@@ -271,6 +273,8 @@ class Test_wrapped_gdf_method:
         """Test if original class is restored if return value subclasses GeoDataFarme and fulfills _check"""
 
         class A(GeoDataFrame):
+            fallback_class = None
+
             @staticmethod
             def _check(obj, validate_geometry=True):
                 return True
@@ -282,66 +286,42 @@ class Test_wrapped_gdf_method:
         a = A(example_positionfixes)
         assert type(foo(a)) is A
 
+    def test_fallback(self, example_positionfixes):
+        """Test if fallback to fallback_class if _check succeeds but not subclasses GeoDataFrame"""
 
-class Test_wrapped_gdf_method_fallback:
-    """Test if _wrapped_gdf_method_fallback conditionals work"""
-
-    def test_no_geodataframe(self, example_positionfixes):
-        """Test if function return value does not subclass DataFrame then __class__ is not touched"""
-
-        @_wrapped_gdf_method_fallback
-        def foo(gdf: GeoDataFrame) -> pd.Series:
-            return pd.Series(gdf.iloc[0])
-
-        assert type(foo(example_positionfixes)) is pd.Series
-
-    def test_failed_check(self, example_positionfixes):
-        """Test if _check fails then __class__ is not touched"""
+        class B(pd.DataFrame):
+            pass
 
         class A(GeoDataFrame):
-            @staticmethod
-            def _check(obj, validate_geometry=True):
-                return False
-
-        @_wrapped_gdf_method_fallback
-        def foo(a: A) -> GeoDataFrame:
-            return GeoDataFrame(a)
-
-        a = A(example_positionfixes)
-        assert type(foo(a)) is GeoDataFrame
-
-    def test_keep_class(self, example_positionfixes):
-        """Test if original class is restored if return value subclasses GeoDataFarme and fulfills _check"""
+            fallback_class = B
 
-        class A(GeoDataFrame):
             @staticmethod
             def _check(obj, validate_geometry=True):
                 return True
 
-        @_wrapped_gdf_method_fallback
-        def foo(a: A) -> GeoDataFrame:
-            return GeoDataFrame(a)
+        @_wrapped_gdf_method
+        def foo(a: A) -> pd.DataFrame:
+            return pd.DataFrame(a)
 
         a = A(example_positionfixes)
-        assert type(foo(a)) is A
+        assert type(foo(a)) is B
 
-    def test_fallback(self, example_positionfixes):
-        class B(pd.DataFrame):
-            pass
+    def test_no_fallback(self, example_positionfixes):
+        """Test if fallback_class is not set then fallback_class is not used."""
 
         class A(GeoDataFrame):
-            fallback_class = B
+            fallback_class = None
 
             @staticmethod
             def _check(obj, validate_geometry=True):
                 return True
 
-        @_wrapped_gdf_method_fallback
+        @_wrapped_gdf_method
         def foo(a: A) -> pd.DataFrame:
             return pd.DataFrame(a)
 
         a = A(example_positionfixes)
-        assert type(foo(a)) is B
+        assert type(foo(a)) is pd.DataFrame
 
 
 class TestTrackintelGeoDataFrame:
@@ -350,49 +330,6 @@ class TestTrackintelGeoDataFrame:
     class A(TrackintelGeoDataFrame):
         """Mimic TrackintelGeoDataFrame subclass by taking the same arguments"""
 
-        def __init__(self, *args, validate_geometry=True, **kwargs):
-            super().__init__(*args, **kwargs)
-
-        @staticmethod
-        def _check(obj, validate_geometry=True):
-            return True
-
-    def test_getitem(self, example_positionfixes):
-        """Test if loc on all columns returns original class."""
-        a = self.A(example_positionfixes)
-        b = a.loc[[True for _ in a.columns]]
-        assert type(b) is self.A
-
-    def test_copy(self, example_positionfixes):
-        """Test if copy maintains class."""
-        a = self.A(example_positionfixes)
-        b = a.copy()
-        assert type(b) is self.A
-
-    def test_merge(self, example_positionfixes):
-        """Test if merge maintains class"""
-        a = self.A(example_positionfixes)
-        b = a.merge(a, on="user_id", suffixes=("", "_other"))
-        assert type(b) is self.A
-
-    def test_constructor_dataframe_fallback(self, example_positionfixes):
-        """Test if _constructor gets DataFrame it falls through"""
-        a = self.A(example_positionfixes)
-        df = example_positionfixes.drop(columns=example_positionfixes.geometry.name)
-        assert type(a._constructor(df)) is pd.DataFrame
-
-    def test_constructor_calls_init(self, example_positionfixes):
-        """Test if _constructor gets GeoDataFrame and fulfills test then builds class"""
-        a = self.A(example_positionfixes)
-        assert type(a._constructor(a)) is self.A
-
-
-class TestTrackintelGeoDataFrameWithFallback:
-    """Test helper class TrackintelGeoDataFrameWithFallback."""
-
-    class A(TrackintelGeoDataFrameWithFallback):
-        """Mimic TrackintelGeoDataFrameWithFallback subclass by taking the same arguments"""
-
         class AFallback(TrackintelDataFrame):
             pass
 
@@ -433,11 +370,18 @@ class TestTrackintelGeoDataFrameWithFallback:
         assert type(a) is GeoDataFrame
 
     def test_constructor_fallback_class(self, example_positionfixes):
-        """Test if _constructor gets DataFrame it falls through"""
+        """Test if _constructor gets can fallback to fallback_class"""
         a = self.A(example_positionfixes)
         a = a.drop(columns=a.geometry.name)
         assert type(a) is self.A.fallback_class
 
+    def test_constructor_no_fallback_class(self, example_positionfixes):
+        """Test if _constructor does not fallback to fallback_class if not set"""
+        a = self.A(example_positionfixes)
+        a.fallback_class = None  # unset it again
+        a = a.drop(columns=a.geometry.name)
+        assert type(a) is pd.DataFrame
+
     def test_constructor_calls_init(self, example_positionfixes):
         """Test if _constructor gets GeoDataFrame and fulfills test then builds class"""
         a = self.A(example_positionfixes)