Add raster support #6

asinghvi17 · 2023-03-27T04:08:59Z

This is a basic implementation of RasterDataSources.jl for NaturalEarth. With this, the code:

using NaturalEarth, Rasters, CairoMakie
# All NaturalEarth rasters are tiffs, 
# so we have to convert them manually to matrices of colors
# This might be better solved in Rasters.jl!
tiff = Raster(NaturalEarthRaster{:latest, 10, :HYP_HR}, 1)
tiff_image = Makie.RGBf.(eachslice(tiff, dims=3)...) ./ 255
image(tiff_image)

just works!

haakon-e

looks like you put in a really great effort on this PR! Thanks for all the time you put into this.

I haven't had the chance to run the code yet, but I'll try to do so one of the next few days.

As a general comment; I'm not sure what kind of tests, if any, would be sensible to write for the code. Perhaps we could put an effort into some documentation in a future PR which downloads and plots some example rasters (and geojson files) to showcase usage. If we set strict==true when building docs, that would hopefully catch most issues, at least every time tests are run.
In that same future PR, we could add some docstrings as well e.g. to the NaturalEarthRaster struct and ne_raster method.

haakon-e · 2023-03-27T04:47:37Z

src/NaturalEarth.jl

+function naturalearth(dataset_name::String)
+    if dataset_name ∈ available_artifacts
+        file_path = @artifact_str("$dataset_name/$dataset_name.geojson")
+        return GeoJSON.read(read(file_path, String))
+    # elseif dataset_name ∈ available_rasters
+    end

+    @assert isfile(file_path) """
+    `$dataset_name` is not a valid NaturalEarth.jl artifact!
+    Please search https://www.naturalearthdata.com for available
+    datasets.
+    """
+end


Is this rewrite related to support for rasters, or just experimentation? :)

Mostly to add support for Rasters in the future...wanted to have a template to build off.

haakon-e · 2023-03-27T04:55:06Z

src/NaturalEarth.jl

+# Implement the RasterDataSources.jl interface.
+# With this, you can call e.g. # Rasters.Raster(NaturalEarthRaster{:latest, 10, :HYP_HR}, nothing)
+
+struct NaturalEarthRaster{Version, Scale, Name} <: RasterDataSources.RasterDataSource end


Is having the version, scale and name as type parameters a stylistic choice, performance gain (vs field names), or something else? .. I'm just curious!

The way RasterDataSources works is that you have to pass a type in to RasterDataSources.getraster, and Rasters.jl has the same dispatch (Raster(T::Type{<: RasterDataSources.RasterDataSource}, layer; kwargs...)).

I do wish we could keep them as fields, since that would allow a lot more validation, but oh well 🤷

I would put less in the type parameter!

RasterDataSources.jl has the layer name as the second argument, and scale would be a keyword.

Otherwise you get compilation every time you use it. Having the name as second argument means a tuple of names can make a stack.

You can put as much of that as you want in the keywords really.

You just need at least one type for dispatch.

To clarify, when there are multiple type parameters in RasterDataSources its usually because the data source is structurally different - they have different keyword arguments and separate dataset handling code. WorldClim{Weather} and WorldClim{Climate} dispatch to totally separate methods in different files.

Here you don't actually use them to dispatch different code, suggesting they probably should be keywords.

There are also a few in RasterDataSources.jl like ALWB that were just design mistakes and have unnecessary things in the type that could be keywords.

haakon-e · 2023-03-27T05:12:31Z

I'm also a little confused at why the tests fail attempting to download the ne_50m_coastline from https://raw.githubusercontent.com/nvkelso/natural-earth-vector/v5.1.2/geojson/ne_50m_coastline.geojson
but I'd think that's unrelated to this PR. That file is one of the ones we didn't mark as lazy, so it should be installed by default when building the package.

asinghvi17 · 2023-03-27T05:19:23Z

Yeah, I'm kind of confused as well...will have to check that out locally.

rafaqz · 2023-03-27T09:38:13Z

src/NaturalEarth.jl

+    return string(Name) * ".tif"
+end
+
+function RasterDataSources.getraster(T::Type{NaturalEarthRaster{Version, Scale, Name}}, layer = nothing) where {Version, Scale, Name}


Couldn't layer be Name ? that's how the rest of RasterDataSources.jl works unless I misunderstand something.

A NTuple{N,Symbol} for layer will then give you a RasterStack automatically, or just a NamedTuple returned from getraster.

ne = NaturalEarth{10 #= scale =#} Raster(ne, :HY_SR)

do you mean something like this, where each type represents a scale, and has its own set of layers we can pull from? That makes sense.

I would just have a scale keyword so there is no recompilation.

Raster(NaturalEarth, :HY_SR; scale=10)

Ok so turns out that wont work! Because Rasters.jl has a fixed list of keywords to pass to getraster which does not include scale.

That's not going to "scale". I think we need a RasterDataSource to declare its keywords to make this more flexible.

Like:

getraster_keywords(NaturalEarth) = (:scale, :version)

Then Rasters.jl can just pass those through and keep the rest.

Yeah, I'd also like to discuss the getraster interface more generally...the reliance on types kind of confused me.

Would love to chat about it at MakieCon :D

With no types at all we have no dispatch. But do you mean types rather than constructed objects?

Partly its just historical, I didnt start the package.

We could rewrite it all to use structs with fields.

Basically I organised and standardised the few initial sources as much as possible, added a few more, and have had very little time to put into it since.

EcoJulia/RasterDataSources.jl#57

rafaqz · 2023-03-27T09:39:51Z

src/NaturalEarth.jl

+
+_zipfile_to_read(raster_name, zf) = first(filter(f -> splitdir(f.name)[2] == raster_name, zf.files))
+
+function RasterDataSources.zippath(::Type{NaturalEarthRaster{Version, Scale, Name}}) where {Version, Scale, Name}


These methods will recompile with any change at all, probably not what you want.

I would guess Version will hardly ever lead to recompilation as people would stick to one version. But then you have the problem of the default, which is easier as a keyword.

Yeah, I didn't pay too much attention to keyword options, so will definitely look at that!

rafaqz · 2023-03-27T09:40:08Z

src/NaturalEarth.jl

+
+struct NaturalEarthRaster{Version, Scale, Name} <: RasterDataSources.RasterDataSource end
+
+function ne_raster(; version = :latest, scale::Int = 10, name::Int) 


Can you instead pass these keywords through and not put them in the type, except maybe at most version? As that's more tied to the definition of the dataset.

For sure. This probably ties in with the question of which dataset to get anyway.

rafaqz · 2023-03-27T09:59:17Z

src/NaturalEarth.jl

+    raster_path = RasterDataSources.rasterpath(T)
+    if !isfile(raster_path)
+        zip_path = RasterDataSources.zippath(T)
+        RasterDataSources._maybe_download(RasterDataSources.zipurl(T), zip_path)


Extending methods with underscores is risky, that's marking them as internal so I can change them in a patch version.

Maybe we should may maybe_download part of the API instead?

That would be nice, but as a utility function I could probably replicate it (I think it's just isfile(zip_path) ? zip_path : download(zipurl, zip_path)?

https://github.com/EcoJulia/RasterDataSources.jl/blob/master/src/shared.jl#L52-L65

src/NaturalEarth.jl

asinghvi17 · 2023-03-27T10:21:45Z

Thanks for the detailed review @rafaqz! I'll definitely look into the layer/kwarg approach.

Co-authored-by: Rafael Schouten <[email protected]>

asinghvi17 added 7 commits January 18, 2023 11:11

Change ownership from asinghvi17 -> JuliaGeo

cf58439

add some docs, error messages

8ac81cb

Zip-unpacking function

db6061d

minor changes for xml

90d5c35

Better grammar in error message

5383272

Basic function to get all available rasters

c62db49

Implement the RasterDataSources.jl interface

9aafdc7

asinghvi17 requested review from rafaqz and haakon-e March 27, 2023 04:08

asinghvi17 added 2 commits March 27, 2023 09:52

Squash some bugs

d52998e

Remove unused code

d700462

haakon-e reviewed Mar 27, 2023

View reviewed changes

rafaqz requested changes Mar 27, 2023

View reviewed changes

Update src/NaturalEarth.jl

fe93f2b

Co-authored-by: Rafael Schouten <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add raster support #6

Add raster support #6

asinghvi17 commented Mar 27, 2023 •

edited

Loading

haakon-e left a comment

haakon-e Mar 27, 2023

asinghvi17 Mar 27, 2023

haakon-e Mar 27, 2023

asinghvi17 Mar 27, 2023

asinghvi17 Mar 27, 2023

rafaqz Mar 27, 2023 •

edited

Loading

rafaqz Mar 27, 2023 •

edited

Loading

haakon-e commented Mar 27, 2023 •

edited

Loading

asinghvi17 commented Mar 27, 2023

rafaqz Mar 27, 2023

asinghvi17 Mar 27, 2023

rafaqz Mar 27, 2023

rafaqz Mar 27, 2023

rafaqz Mar 27, 2023

asinghvi17 Mar 27, 2023 •

edited

Loading

rafaqz Mar 27, 2023 •

edited

Loading

rafaqz Mar 27, 2023

rafaqz Mar 27, 2023

rafaqz Mar 27, 2023

asinghvi17 Mar 27, 2023

rafaqz Mar 27, 2023

asinghvi17 Mar 27, 2023

rafaqz Mar 27, 2023

asinghvi17 Mar 27, 2023

rafaqz Mar 27, 2023

asinghvi17 commented Mar 27, 2023


		_zipfile_to_read(raster_name, zf) = first(filter(f -> splitdir(f.name)[2] == raster_name, zf.files))

		function RasterDataSources.zippath(::Type{NaturalEarthRaster{Version, Scale, Name}}) where {Version, Scale, Name}


		struct NaturalEarthRaster{Version, Scale, Name} <: RasterDataSources.RasterDataSource end

		function ne_raster(; version = :latest, scale::Int = 10, name::Int)

Add raster support #6

Are you sure you want to change the base?

Add raster support #6

Conversation

asinghvi17 commented Mar 27, 2023 • edited Loading

haakon-e left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

rafaqz Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

haakon-e commented Mar 27, 2023 • edited Loading

asinghvi17 commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asinghvi17 Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

rafaqz Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asinghvi17 commented Mar 27, 2023

asinghvi17 commented Mar 27, 2023 •

edited

Loading

rafaqz Mar 27, 2023 •

edited

Loading

rafaqz Mar 27, 2023 •

edited

Loading

haakon-e commented Mar 27, 2023 •

edited

Loading

asinghvi17 Mar 27, 2023 •

edited

Loading

rafaqz Mar 27, 2023 •

edited

Loading