Skip to content

Fix logic for grouping consecutive points in CountriesAndCities #610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 29, 2025

Conversation

arne182
Copy link
Contributor

@arne182 arne182 commented Jan 1, 2025

This update corrects the logic for grouping consecutive points in the group_points_with_consecutive_cities method. It ensures sessions are properly split when transitioning between cities or encountering significant time gaps, leading to accurate grouping and filtering of points based on session duration.
This problem had citys showning up in the list that were driven through with over an apart but points not consequtively in that city.

This update corrects the logic for grouping consecutive points in the group_points_with_consecutive_cities method. It ensures sessions are properly split when transitioning between cities or encountering significant time gaps, leading to accurate grouping and filtering of points based on session duration.
Update to check the fixed logic
@Freika Freika changed the base branch from master to dev May 29, 2025 10:38
@Freika Freika requested a review from Copilot May 29, 2025 10:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the logic for grouping consecutive points by addressing inconsistencies in the session splitting mechanism in the CountriesAndCities service. Key changes include updating the test cases in the spec file to reflect the new expected behavior, modifying the call method to only include countries with valid sessions, and introducing a new grouping method for consecutive points.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
spec/services/countries_and_cities_spec.rb Updated test cases with revised expectations and point data
app/services/countries_and_cities.rb Revised session grouping logic and filtering implementation


build_city_data(city, points_count, timestamps, duration)
# Split session if city changes or time gap exceeds the threshold
if point.city != prev_point.city
Copy link
Preview

Copilot AI May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The session splitting condition checks only for a change in city, but the comment indicates that sessions should also split when a time gap exceeds a threshold. Consider adding a condition to compare the time difference between the current point and the previous point.

Suggested change
if point.city != prev_point.city
if point.city != prev_point.city || (point.timestamp - prev_point.timestamp) > MAX_TIME_GAP

Copilot uses AI. Check for mistakes.

Copy link
Owner

@Freika Freika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@Freika Freika merged commit 05018b6 into Freika:dev May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants