fix: validate coordinates with isValidLatLon before S2 clustering

[Vishmayraj]

The getClusterID function was producing S2 cluster IDs for stops with coordinates (0,0), which is typically a placeholder for improperly formatted GTFS data.

I noticed that the geo package already includes a validation check with the function “isValidLatLon,” but this is not currently used in the S2 clustering path for the aforementioned stops.

This PR includes the validation check in the 0 and 4 cases of the S2 fallback path, as well as a test to ensure that the previous incorrect behavior is now working as expected.

[0xaboomar]

Hi @Vishmayraj , thanks for the PR and for taking a close look at this.

The current behavior of getClusterID() is correct. Since Stop.Latitude and Stop.Longitude are pointers, we check whether they are set by verifying they are not nil.

The two lines you’re attempting to change already correctly represent the intended behavior, as clarified in the comments:

• Line 46: If a stop has no parent but has lat/lon, it is clustered by S2 cell.
• Line 64: If a parent exists but the grandparent is missing, we fall back to clustering by S2 using the stop’s lat/lon.

That said, you’ve raised a good point regarding how we treat coordinates (0, 0) as invalid. I’ll revisit this decision and verify whether it still makes sense. From a quick check, it doesn’t appear to be used elsewhere, but I’ll investigate further before deciding whether to keep or remove this constraint.

Thanks again for the thoughtful observation.

[0xaboomar]

Thanks for the PR, @Vishmayraj. Please take a look at the requested changes and let me know once they’re updated.

As an additional improvement, you could update the parameters of isValidLatLon() to use *float64 and adjust the callers accordingly. This additional improvement isn’t a blocker for merging, so we can proceed without it and address it later.

[0xaboomar]

I recommend moving the guard isValidLatLon() and the check of lat and lon equal nil before the switch case so we reject malformed stops early.

[0xaboomar]

Update the test to make sure that malformed stops (ones with later lat | lon or lat not between (-90 , 90) or lon not between (-180 , 180)) is rejected. Skip the (0,0) checking because we will change this making isValidLatLon() depends on *float type to check whether lat/lon exist or not.

[aaronbrethorst]

Code review

Found 1 issue:

1. The behavior change is not reflected in getClusterID's doc comment, which is the file's spec for exactly these branches. It still says a no-parent stop "has lat/lon, cluster by S2 cell" and that only "no coordinates" / "coordinates are missing" is malformed — but after this change a stop that has coordinates of (0,0) (or out-of-range values) is now treated as malformed and dropped. The same gap exists in the location_type = 4 block (L64-L65). This matters here because those comment lines are precisely what the reviewer cited as the intended contract; on this file a prior review already asked that clustering decisions be documented inline ("Can you add a comment to the codebase here indicating why this decision was made the way it was?", PR feat: add hybrid geo-clustering for unmatched GTFS stops using station and S2 logic #71).

watchdog/internal/geo/geo_cluster.go

Lines 45 to 48 in aa709b6

	// - Invalid: parent exists but is not of type 1.
	// - If it has no parent but has lat/lon, cluster by S2 cell.
	// - If it has no parent and no coordinates, data is malformed.
	//

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}