EcoJulia / RasterDataSources.jl

Easily download and use raster data sets in Julia
MIT License
21 stars 10 forks source link

Add SRTM dataset #46

Closed jenkspt closed 2 years ago

jenkspt commented 2 years ago

This is a first attempt at adding the SRTM dataset

Some notes on the implementation:

To handle these constraints, getraster takes a bounds argument for selecting the dataset region to download. When called with the bounds argument, this method also returns a matrix of strings with the appropriate tile paths.

TODO: Need to propose changes so that Raster(SRTM, bounds, ...) from Rasters.jl can be implemented

jenkspt commented 2 years ago

Is there a reason the CI is running with Julia 1.5? is it the [raster_path1;;] syntax that's causing the error?

rafaqz commented 2 years ago

Thanks for the PR! This looks great.

The reason for testing on 1.5 and any errors is simply that I'm spread too thin over other packages and havent had time to work on this as much as I would like to.

Github actions previously didn't recognise just "1" so you needed the specific version. But it does now, so we should use that and always test on the latest version.

jenkspt commented 2 years ago

I believe 1.6.5 is now the long term release. From the downloads page https://julialang.org/downloads/

jenkspt commented 2 years ago

Should rasterpath and rastername also have methods with the bounds argument?

rafaqz commented 2 years ago

Yep, and tile_indices also has to be a keyword arg everywhere.

The second argument to getraster is always the layer name/s. If there are no layers, there probably should be no second argument.

Otherwise multiple tiles means a RasterStack

rafaqz commented 2 years ago

We can test on 1.6 or just 1, I dont mind. I havent tested multiple versions because CI downloads a lot of files.

jenkspt commented 2 years ago

Ok so I removed the resolution and defres methods, and added bounds and tile_index keyword arguments to all of the relevant functions.

Tests are passing locally for me with Julia 1.7.1 -- not exactly sure why the CI is failing.

rafaqz commented 2 years ago

Have you missed pushing some updates to your tests?

https://github.com/EcoJulia/RasterDataSources.jl/runs/4833591681?check_suite_focus=true#step:5:1446

codecov[bot] commented 2 years ago

Codecov Report

Merging #46 (46ef205) into master (f19f11f) will increase coverage by 1.58%. The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   73.41%   75.00%   +1.58%     
==========================================
  Files          15       16       +1     
  Lines         425      464      +39     
==========================================
+ Hits          312      348      +36     
- Misses        113      116       +3     
Impacted Files Coverage Δ
src/RasterDataSources.jl 100.00% <ø> (ø)
src/srtm/srtm.jl 92.30% <92.30%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f19f11f...46ef205. Read the comment docs.

rafaqz commented 2 years ago

Thanks! Turns out I need to use this pretty much straight away in my own work, so looking forward to testing it out.