Skip to content
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

Use geometry rather than geom #50

Closed
rafaqz opened this issue May 11, 2023 · 6 comments · Fixed by #69
Closed

Use geometry rather than geom #50

rafaqz opened this issue May 11, 2023 · 6 comments · Fixed by #69
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@rafaqz
Copy link
Member

rafaqz commented May 11, 2023

GeoInterface.jl defaults to tables using geometry as the geometry column name. It would be nice if GADM.jl tables just worked in e.g. Rasters.jl without manually specifying the geometry column or getting them out.

See:
https://github.com/JuliaGeo/GeoInterface.jl/blob/main/src/interface.jl#L47

This default can be overridden with a custom table type in GeoInterface.geometrycolumns. But for NamedTuple tables
that requires type pyracy, so using the default column name is the way to get compatibility.

@juliohm juliohm added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels May 11, 2023
@eliascarv
Copy link
Collaborator

@rafaqz, the name geom is defined by the ArchGDAL.jl package.
The GADM.j code just converts the data returned by ArchGDAL.jl into a table using the Tables.columntable function.

@rafaqz
Copy link
Member Author

rafaqz commented May 16, 2023

Its probably defined in the dataset itself, but gdal passes us :geom in GeoInterface.geometrycolumns so it can be called whatever they like.

GADM.jl strips that information by converting the table with Tables.columntable.

Without changing the column name to the generic one at the same time you lose ecosystem compatability.

Another option would be to define a wrapper table that knows which column has the geometries.

@juliohm
Copy link
Member

juliohm commented May 16, 2023

Its probably defined in the dataset itself, but gdal passes us :geom in GeoInterface.geometrycolumns so it can be called whatever they like.

What is your suggestion then? We simply get whatever column name that GDAL returns to us and move it forward. I don't see what we could do differently? Why we need an extra wrapper table type if ArchGDAL.jl already provides that for us?

@rafaqz
Copy link
Member Author

rafaqz commented May 16, 2023

But youre not returning the ArchGDAL table... thats where the problem comes from. If you did it would work, and would be ArchGDALs problem if it didnt.

From my perspective, the options are

  1. Return the table as-is from ArchGDAL (probably deleting 1 line of code)
  2. Change the column name to geometry in the conversion process (maybe adding 2 lines of code)
  3. Add your own wrapper table (probably annoying)
  4. Leave things as they are, and everyone has to manually do table.geom.

It would be nice not to do 4, its a bit confusing to new users to see that in code everywhere.

@rafaqz
Copy link
Member Author

rafaqz commented May 16, 2023

A fifth options is to leverage the new metadata interface for tables, and add the column name to metadata so it propagates everywhere.

But its some serious work to implement that in GeoInterface and in every table producing geo package, and someone would need to put their hand up to do it.

Edit: And I dont think it propagates to the table youre returning anyway, so probably not an option.

@juliohm
Copy link
Member

juliohm commented Jun 14, 2023

2. Change the column name to geometry in the conversion process (maybe adding 2 lines of code)

We should probably go with this option. If someone starts working on it, please comment on the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants