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

Add support for GeoInterface CRS metadata #25

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

eliascarv
Copy link
Contributor

@eliascarv eliascarv commented Oct 3, 2024

Waiting for JuliaGeo/GeoInterface.jl#161
Closes #24

Test code:

julia> using GeoParquet, DataFrames

julia> df = GeoParquet.read("example.parquet")
5×6 DataFrame
 Row │ pop_est         continent      name                      iso_a3   gdp_md_est  geometry                          
     │ Float64?        String?        String?                   String?  Int64?      WellKnow                         
─────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1889953.0        Oceania        Fiji                      FJI            5496  WellKnownBinary{Geom, Vector{UIn
                                                                                     
                                                                                                         4 rows omitted

julia> metadata(df, "GEOINTERFACE:geometrycolumns")
(:geometry,)

julia> metadata(df, "GEOINTERFACE:crs")
GeoFormatTypes.ProjJSON(...)

src/io.jl Outdated Show resolved Hide resolved
@eliascarv eliascarv requested a review from rafaqz October 3, 2024 21:08
src/io.jl Outdated Show resolved Hide resolved
@eliascarv eliascarv requested a review from rafaqz October 3, 2024 21:13
@asinghvi17
Copy link
Member

Does the ProjJSON type work with a Dict? I'm pretty sure it would have to be a String...at least to be passed to GDAL.

src/io.jl Show resolved Hide resolved
@eliascarv
Copy link
Contributor Author

eliascarv commented Oct 4, 2024

Yes, ProjJSON type supports Dicts and Strings:
https://github.com/JuliaGeo/GeoFormatTypes.jl/blob/master/src/GeoFormatTypes.jl#L193-L213

@eliascarv
Copy link
Contributor Author

And GeoParquet.write does not works very well when ProjJSON is passed as String:

julia> using GeoParquet, Parquet2, DataFrames, JSON3, GeoFormatTypes

julia> df = GeoParquet.read("example.parquet")
5×6 DataFrame
 Row │ pop_est         continent      name                      iso_a3   gdp_md_est  geometry                          
     │ Float64?        String?        String?                   String?  Int64?      WellKnow                         
─────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1889953.0        Oceania        Fiji                      FJI            5496  WellKnownBinary{Geom, Vector{UIn
                                                                                     
                                                                                                         4 rows omitted

julia> crs = metadata(df, "GEOINTERFACE:crs");

julia> jsonstr = JSON3.write(GeoFormatTypes.val(crs));

julia> newcrs = ProjJSON(jsonstr);

julia> GeoParquet.write("test.parquet", df, (:geometry,), newcrs)
"test.parquet"

julia> GeoParquet.read("test.parquet")
ERROR: ArgumentError: Not a ProjJSON: ...
julia> ds = Parquet2.Dataset("test.parquet");

julia> meta = JSON3.read(Parquet2.metadata(ds)["geo"]);

julia> meta.columns.geometry.crs
JSON3.Object{Base.CodeUnits{UInt8, String}, SubArray{UInt64, 1, Vector{UInt64}, Tuple{UnitRange{Int64}}, true}} with 1318 entries:
  Symbol("1") => "{"
  Symbol("2") => "\""
  Symbol("3") => "\$"
  Symbol("4") => "s"
  Symbol("5") => "c"
             => 

@rafaqz
Copy link
Member

rafaqz commented Oct 4, 2024

Looks like a bug

@eliascarv
Copy link
Contributor Author

This happens because the GeoParquet.jl implementation treats ProjJSON as a dictionary:

image

Unfortunately, I don't know what the motivations for this implementation were.

@rafaqz
Copy link
Member

rafaqz commented Oct 4, 2024

That's also type piracy!

@eliascarv
Copy link
Contributor Author

eliascarv commented Oct 4, 2024

Yes, my PR doesn't solve these issues, but for now, I think it's enough. These issues can be solved in other PRs.
What do you think?

@asinghvi17
Copy link
Member

asinghvi17 commented Oct 5, 2024

Looks good to me for now, would be nice to add a test though. Maybe the code you have in the README would suffice for that?

Long term we need better geometry support, since WKB geoms won't have attached CRS for example. But that can wait for later.

@eliascarv
Copy link
Contributor Author

@asinghvi17, tests added.

@evetion evetion merged commit 46f1b35 into JuliaGeo:main Dec 20, 2024
6 of 12 checks passed
@evetion
Copy link
Member

evetion commented Dec 20, 2024

Thanks @eliascarv, sorry for the delay in merging. I will do some light maintenance here and make a release soon.

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.

Add support for GI.crs
4 participants