Skip to content

Conversation

@zhangfengcdt
Copy link
Member

@zhangfengcdt zhangfengcdt commented Apr 12, 2025

Did you read the Contributor Guide?

Is this PR related to a ticket?

  • Yes, and the PR name follows the format [SEDONA-XXX] my subject.

What changes were proposed in this PR?

This PR refactor the GeoSeries python wrapper so that the code can be simplified, e.g.,

    def area(self) -> "GeoSeries":
           return self._process_geometry_column("ST_Area", rename="area")

    def buffer(
        self,
        distance,
        resolution=16,
        cap_style="round",
        join_style="round",
        mitre_limit=5.0,
        single_sided=False,
        **kwargs,
    ):
        return self._process_geometry_column(
            "ST_Buffer", rename="buffer", distance=distance
        )

How was this patch tested?

test_geoseries.py

Did this PR include necessary documentation updates?

  • No, this PR does not affect any public API so no need to change the documentation.

@zhangfengcdt zhangfengcdt marked this pull request as ready for review April 12, 2025 18:42
@zhangfengcdt zhangfengcdt requested a review from jiayuasu as a code owner April 12, 2025 18:42
@jiayuasu jiayuasu requested review from Copilot and furqaankhan April 12, 2025 19:04
Copy link
Contributor

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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

python/sedona/geopandas/geoseries.py:172

  • [nitpick] Consider renaming the variable 'all_args' (which aggregates both positional and keyword parameters) to 'all_params' to more clearly indicate its purpose.
for k, v in kwargs.items():

Copy link
Contributor

@furqaankhan furqaankhan left a comment

Choose a reason for hiding this comment

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

LGTM!

One QQ: What if the user wants to use any other geometry column than the first one in the schema? Because I see that we take the very first geometry column as input to the functions. Should we add a field that lets users pick the geometry column? This may not be the scope for this PR.

@zhangfengcdt
Copy link
Member Author

LGTM!

One QQ: What if the user wants to use any other geometry column than the first one in the schema? Because I see that we take the very first geometry column as input to the functions. Should we add a field that lets users pick the geometry column? This may not be the scope for this PR.

The GeoSeries (and Pandas Series) should only have one "column" that in geo should be geometry type. Though for GeoDataFrame (and Pandas DataFrame) they can have multiple geometry columns. We can use set_geometry on the dataframe, but it won't be necessary (needed) for GeoSeries.

@furqaankhan
Copy link
Contributor

furqaankhan commented Apr 14, 2025

Understood, thank you!

I think we should use either set_geometry or have an optional parameter to specify which geometry column they want to operate on. Because from what I can understand, the current GeoDataFrame implementation of _process_geometry_columns would process all geometry columns in the GeoDataFrame, right?

@jiayuasu jiayuasu added this to the sedona-1.8.0 milestone Apr 14, 2025
@jiayuasu jiayuasu merged commit 146bb7d into apache:master Apr 14, 2025
26 checks passed
Kontinuation pushed a commit to Kontinuation/sedona that referenced this pull request Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants