Skip to content

Conversation

asinghvi17
Copy link
Member

what it says on the tin

we should phase out the `geocolumn` optional argument in favour of the keyword argument, this is backwards compatible and nonbreaking but we should, at some point, raise a depwarn and then make a breaking release that removes that argument.
@evetion
Copy link
Member

evetion commented Apr 24, 2025

Uhm, there's geocolumns=GI.geometrycolumns(df) right there no? The keyword should be renamed, but the functionality is there.

@asinghvi17
Copy link
Member Author

That's a positional argument, for consistency I'm trying to make everything a kwarg

@evetion
Copy link
Member

evetion commented Apr 27, 2025

Fair enough, but let's deprecate this signature then, and only have a kwarg in the new one. And do you prefer geometrycolumns or geocolumns (or variations thereof with _)? GDF does geom_columns, terrible. But at least GDF will have an extension on GeoParquet here.

@rafaqz
Copy link
Member

rafaqz commented Apr 27, 2025

Let's standardise these keyword everywhere, we really need to write that common interface docstring in GeoInterface...

geometrycolumn is correct and clear and what Rasters users, but it's too long. geocolumn could be better?

Also are you thinking plural or single?

@asinghvi17
Copy link
Member Author

asinghvi17 commented Apr 27, 2025

Plural needs to be supported, @lazarusA's tutorial has a case where you have the centroid of a geom and the geom itself in a dataframe, which both would need to be transformed / reprojected at the same time.

But the kwarg can be singular, I don't have any particular bias here or there.

The only thing is that it needs to accept kwarg = :colname and kwarg = (:col1, :col2) and deal with them appropriately, as well as using GI.geometrycolumns as a fallback.

@evetion
Copy link
Member

evetion commented Apr 27, 2025

I prefer geometrycolumns. Note that GDF doesn't accept a Symbol, it has to be a tuple for this argument.

It's longer, but with the defaults it shouldn't be used that much.

@rafaqz
Copy link
Member

rafaqz commented Apr 27, 2025

I really prefer singlular and accepting a Symbol, as it's the 95% use case. And e.g. Rasters can only accept one column to rasterize at a time so can't use the plural, but no package will only accept multiple columns.

@asinghvi17
Copy link
Member Author

Note that GDF doesn't accept a Symbol

We should probably change that, it's pretty annoying on the user end IMO. And tough to tell someone they have to pass in a tuple when we can detect and wrap.

the 95% use case

Definitely - but I would argue that with the metadata support now it's not as important as it used to be. And if you're creating a table from scratch anyway, then it's not so hard to add an extra character at the end...

@rafaqz
Copy link
Member

rafaqz commented Apr 28, 2025

Just to clarify my reason to want geometrycolumn without the s is to standardise everything with Rasters, which can't be plural because you can only pass one Symbol.

I could change it to geocolumn etc or whatever we choose, but not with an s on the end.

@asinghvi17
Copy link
Member Author

FWIW I ended up (before this) using geometrycolumn in GeometryOps: https://github.com/JuliaGeo/GeometryOps.jl/pull/302/files#diff-e4af6fa0abadc0660c1c020b88ec02ba3ac2eaceb0b0ff10d2d27d6e04d03700R178

It does flow more naturally especially for the 99% case of a single geometry column.

@asinghvi17 asinghvi17 requested a review from evetion May 15, 2025 21:47
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.

3 participants