Repository: mie-lab/trackintel

Issue:
BUG: generate_staypoints and generate_trips
if the input pfs already has a column `staypoint_id`, the following error will occur on Line 144

`ValueError: columns overlap but no suffix specified: Index(['staypoint_id'], dtype='object')`

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/mie-lab/trackintel/pull/217?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab) Report
> Merging [#217](https://codecov.io/gh/mie-lab/trackintel/pull/217?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab) (e25ed42) into [master](https://codecov.io/gh/mie-lab/trackintel/commit/ddd0a9df13d5e4e0ffe962ccf261655e1c814aa4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab) (ddd0a9d) will **decrease** coverage by `4.65%`.
> The diff coverage is `0.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
- Coverage   94.70%   90.04%   -4.66%     
==========================================
  Files          36       36              
  Lines        1473     1477       +4     
  Branches      244      246       +2     
==========================================
- Hits         1395     1330      -65     
- Misses         41      108      +67     
- Partials       37       39       +2     
```


| [Impacted Files](https://codecov.io/gh/mie-lab/trackintel/pull/217?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab) | Coverage Δ | |
|---|---|---|
| [trackintel/preprocessing/positionfixes.py](https://codecov.io/gh/mie-lab/trackintel/pull/217/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9wcmVwcm9jZXNzaW5nL3Bvc2l0aW9uZml4ZXMucHk=) | `93.37% <0.00%> (-2.55%)` | :arrow_down: |
| [trackintel/io/postgis.py](https://codecov.io/gh/mie-lab/trackintel/pull/217/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9pby9wb3N0Z2lzLnB5) | `17.80% <0.00%> (-82.20%)` | :arrow_down: |
| [trackintel/model/trips.py](https://codecov.io/gh/mie-lab/trackintel/pull/217/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9tb2RlbC90cmlwcy5weQ==) | `95.23% <0.00%> (-4.77%)` | :arrow_down: |
| [trackintel/model/locations.py](https://codecov.io/gh/mie-lab/trackintel/pull/217/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9tb2RlbC9sb2NhdGlvbnMucHk=) | `96.00% <0.00%> (-4.00%)` | :arrow_down: |
| [trackintel/model/positionfixes.py](https://codecov.io/gh/mie-lab/trackintel/pull/217/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9tb2RlbC9wb3NpdGlvbmZpeGVzLnB5) | `97.05% <0.00%> (-2.95%)` | :arrow_down: |
| [trackintel/model/triplegs.py](https://codecov.io/gh/mie-lab/trackintel/pull/217/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9tb2RlbC90cmlwbGVncy5weQ==) | `97.22% <0.00%> (-2.78%)` | :arrow_down: |
| [trackintel/model/staypoints.py](https://codecov.io/gh/mie-lab/trackintel/pull/217/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mie-lab#diff-dHJhY2tpbnRlbC9tb2RlbC9zdGF5cG9pbnRzLnB5) | `97.22% <0.00%> (-2.78%)` | :arrow_down: |

------

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

abcnishant007: @hong2223 
I added two tests to make sure that the actual bug does not exist anymore. 
do you have any suggestions why my the code coverage decreased after my changes? 
abcnishant007: @hong2223 ignore the previous comment about coverage. Actually the coverage decrease was probably because of some older results being displayed. It is fine now. Please feel free to review.
hong2223: > @hong2223 once the contributor has made the requested changes according to the review, is the contributor supposed to click on "Resolve conversation" for each sub-comment? I just wrote "done" in each, not sure if that is the right way 😆

As you wish, we are flexible on this. I will now merge this PR, great work!

Patch:
diff --git a/trackintel/preprocessing/positionfixes.py b/trackintel/preprocessing/positionfixes.py
index 650c5a3..46afbf9 100644
--- a/trackintel/preprocessing/positionfixes.py
+++ b/trackintel/preprocessing/positionfixes.py
@@ -85,6 +85,10 @@ def generate_staypoints(
     # copy the original pfs for adding 'staypoint_id' column
     pfs = pfs_input.copy()
 
+    # if the positionfixes already have a column "staypoint_id", we drop it
+    if "staypoint_id" in pfs:
+        pfs.drop(columns="staypoint_id", inplace=True)
+
     elevation_flag = "elevation" in pfs.columns  # if there is elevation data
 
     geo_col = pfs.geometry.name
@@ -214,6 +218,10 @@ def generate_triplegs(pfs_input, stps_input, method="between_staypoints", gap_th
     # copy the original pfs for adding 'staypoint_id' column
     pfs = pfs_input.copy()
 
+    # if the positionfixes already have a column "tripleg_id", we drop it
+    if "tripleg_id" in pfs:
+        pfs.drop(columns="tripleg_id", inplace=True)
+
     if method == "between_staypoints":
 
         # get case:
diff --git a/trackintel/preprocessing/triplegs.py b/trackintel/preprocessing/triplegs.py
index 066c95b..f599a38 100644
--- a/trackintel/preprocessing/triplegs.py
+++ b/trackintel/preprocessing/triplegs.py
@@ -91,6 +91,15 @@ def generate_trips(stps_input, tpls_input, gap_threshold=15, print_progress=Fals
     tpls = tpls_input.copy()
     stps = stps_input.copy()
 
+    # if the triplegs already have a column "trip_id", we drop it
+    if "trip_id" in tpls:
+        tpls.drop(columns="trip_id", inplace=True)
+
+    # if the staypoints already have any of the columns  "trip_id", "prev_trip_id", "next_trip_id", we drop them
+    for col in ["trip_id", "prev_trip_id", "next_trip_id"]:
+        if col in stps:
+            stps.drop(columns=col, inplace=True)
+
     tpls["type"] = "tripleg"
     stps["type"] = "staypoint"
 


Test patch:
diff --git a/tests/preprocessing/test_positionfixes.py b/tests/preprocessing/test_positionfixes.py
index b4664ca..e16b9a9 100644
--- a/tests/preprocessing/test_positionfixes.py
+++ b/tests/preprocessing/test_positionfixes.py
@@ -23,6 +23,20 @@ def geolife_pfs_stps_long():
 class TestGenerate_staypoints:
     """Tests for generate_staypoints() method."""
 
+    def test_duplicate_columns(self):
+        """Test if running the function twice, the generated column does not yield exception in join statement"""
+
+        # we run generate_staypoints twice in order to check that the extra column(tripleg_id) does
+        # not cause any problems in the second run
+        pfs, _ = ti.io.dataset_reader.read_geolife(os.path.join("tests", "data", "geolife"))
+        pfs_run_1, _ = pfs.as_positionfixes.generate_staypoints(
+            method="sliding", dist_threshold=100, time_threshold=5.0, include_last=True
+        )
+        pfs_run_2, _ = pfs_run_1.as_positionfixes.generate_staypoints(
+            method="sliding", dist_threshold=100, time_threshold=5.0, include_last=True
+        )
+        assert set(pfs_run_1.columns) == set(pfs_run_2.columns)
+
     def test_sliding_min(self):
         """Test if using small thresholds, stp extraction yields each pfs."""
         pfs, _ = ti.io.dataset_reader.read_geolife(os.path.join("tests", "data", "geolife"))
@@ -123,6 +137,17 @@ class TestGenerate_staypoints:
 class TestGenerate_triplegs:
     """Tests for generate_triplegs() method."""
 
+    def test_duplicate_columns(self, geolife_pfs_stps_long):
+        """Test if running the function twice, the generated column does not yield exception in join statement"""
+
+        # we run generate_triplegs twice in order to check that the extra column (tripleg_id) does
+        # not cause any problems in the second run
+        pfs, stps = geolife_pfs_stps_long
+
+        pfs_run_1, _ = pfs.as_positionfixes.generate_triplegs(stps, method="between_staypoints")
+        pfs_run_2, _ = pfs_run_1.as_positionfixes.generate_triplegs(stps, method="between_staypoints")
+        assert set(pfs_run_1.columns) == set(pfs_run_2.columns)
+
     def test_user_without_stps(self, geolife_pfs_stps_long):
         """Check if it is safe to have users that have pfs but no stps."""
         pfs, stps = geolife_pfs_stps_long
diff --git a/tests/preprocessing/test_triplegs.py b/tests/preprocessing/test_triplegs.py
index 76d793a..40e3388 100644
--- a/tests/preprocessing/test_triplegs.py
+++ b/tests/preprocessing/test_triplegs.py
@@ -29,6 +29,24 @@ class TestSmoothen_triplegs:
 class TestGenerate_trips:
     """Tests for generate_trips() method."""
 
+    def test_duplicate_columns(self):
+        """Test if running the function twice, the generated column does not yield exception in join statement"""
+        # load pregenerated trips
+        trips_loaded = ti.read_trips_csv(os.path.join("tests", "data", "geolife_long", "trips.csv"), index_col="id")
+
+        # create trips from geolife (based on positionfixes)
+        pfs, _ = ti.io.dataset_reader.read_geolife(os.path.join("tests", "data", "geolife_long"))
+        pfs, stps = pfs.as_positionfixes.generate_staypoints(method="sliding", dist_threshold=25, time_threshold=5)
+        stps = stps.as_staypoints.create_activity_flag(time_threshold=15)
+        pfs, tpls = pfs.as_positionfixes.generate_triplegs(stps)
+
+        # generate trips and a joint staypoint/triplegs dataframe
+        stps_run_1, tpls_run_1, _ = ti.preprocessing.triplegs.generate_trips(stps, tpls, gap_threshold=15)
+        stps_run_2, tpls_run_2, _ = ti.preprocessing.triplegs.generate_trips(stps_run_1, tpls_run_1, gap_threshold=15)
+
+        assert set(tpls_run_1.columns) == set(tpls_run_2.columns)
+        assert set(stps_run_1.columns) == set(stps_run_2.columns)
+
     def test_generate_trips(self):
         """Test if we can generate the example trips based on example data."""
         # load pregenerated trips