chay0112 commented on PR #2488: URL: https://github.com/apache/sedona/pull/2488#issuecomment-3515133746
> Now, let's address the [CI failure](https://github.com/apache/sedona/actions/runs/19247675887/job/55025776659) of your original commit. The reality is that the issue was in the test itself. I could explain why it was causing problems, but honestly it's a little complicated. > > ```python > # 2) Coverage parity — compare only where inputs are valid for both backends > nonnull_nonempty_ps = (gs_in.isna() == False) & (gs_in.is_empty == False) > ``` > > Instead, I'd like you take a step back and rewrite the test. Don't overthink it. You honestly really don't need to understand Sedona as a project much to contribute these Geopandas functions. A lot of it is just copy-paste and following patterns. Now I'll ask you to do this: > > 1. Go to `test_match_geopandas_series.py`, and delete all of the contents of the `test_minimum_bounding_circle` test. > 2. Take a look at the following functions in `test_match_geopandas_series.py`. Then, guess how you should implement `test_minimum_bounding_circle`. It's very simple, much simpler than the original test in this PR. You shouldn't need to ask AI at all. You're welcome to copy and paste. > > https://github.com/apache/sedona/blob/762d6f85004c17b3bafef73dc3bb65a3a058e38d/python/tests/geopandas/test_match_geopandas_series.py#L721-L725 > > If you're still confused, you're welcome to ask for help. We might need to make some small adjustments to make things pass, but this is a lot closer to what our desired end result is. Thanks for the great suggestion! I tried a similar approach to what’s used in the centroid test. ``` def test_minimum_bounding_circle(self): for geom in self.geoms: sgpd_result = GeoSeries(geom).minimum_bounding_circle() gpd_result = gpd.GeoSeries(geom).minimum_bounding_circle() self.check_sgpd_equals_gpd(sgpd_result, gpd_result) ``` The above test fails because the tolerance level in the check_sgpd_equals_gpd method is currently set to 1e-2. However, increasing the tolerance to 0.5 allows the test to pass. Would you prefer that I update the tolerance to 0.5 in the check_sgpd_equals_gpd method, or handle this case-specific adjustment directly within my test, as shown below ? or am I completely wrong about the analogy ? ``` def test_minimum_bounding_circle(self): for geom in self.geoms: sgpd_result = GeoSeries(geom).minimum_bounding_circle() gpd_result = gpd.GeoSeries(geom).minimum_bounding_circle() for s, g in zip(sgpd_result.to_pandas(), gpd_result): assert s.is_valid and g.is_valid assert abs(s.area - g.area) < 0.5 ``` No rush ! Please take your time to reply. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
