search

JuliaGeo / GADM.jl

A Julia package for obtaining geographical data from the GADM dataset
https://gadm.org
MIT License
38 stars 4 forks source link

Use `geometry` rather than `geom` #50

Closed rafaqz closed 1 month ago

rafaqz commented 1 year ago

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.

eliascarv commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.