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 resample wrapper and methods to convert between ArchGDAL.Dataset and GeoArray #73

Merged
merged 14 commits into from
Oct 14, 2020

Conversation

vlandau
Copy link
Collaborator

@vlandau vlandau commented Oct 10, 2020

Closes #68. Still a WIP -- no formal tests or docs yet, but can confirm that it is working for resampling a single-band raster from 1km resolution in a UTM projection (left panel in figure below) to 0.002° in EPSG:4326 (right panel in figure below) 🎉. I think it should work for multi-band rasters. An edge case that would have errors would be if different bands have different NoData values, but I'm not sure if GeoData handles that to start.

I had some issues/trouble converting ArchGDAL.Datasets and RasterDatasets to GDALarrays, so I just skipped the middle man since the ArchGDAL.Dataset is being created in memory anyway. I added a method to convert back and forth between ArchGDAL.Dataset and GeoData.GeoArray.

@rafaqz I wanted to give you a chance to review what I have so far before proceeding further.

image image

Copy link
Owner

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice, glad it came together fairly smoothly.

src/sources/gdal.jl Outdated Show resolved Hide resolved
function unsafe_ArchGDALdataset(A::GeoArray)
width = size(A, Lon)
height = size(A, Lat)
nbands = size(A, Band)
Copy link
Owner

@rafaqz rafaqz Oct 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't actually be sure that a GeoArray has any of these dimensions - especially Band but the others as well as it could have been sliced. You can use e.g. if hasdim(A, Lon) to check if A has a Lat dim.

You can make a Dataset from 2d without a Band - check the write methods.

The GeoArray may also have other dimensions (that you can get with otherdims(A, (Lat, Lon, Band))) I was wondering if it would be cool to just loop over the time/vertical etc dimension somewhere and slice it - then convert each slice with GDAL and cat them together afterwards. But don't worry about that now :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see -- is there a way to identify the X dimension, the Y dimension, and the number of bands without relying on dim names?

Copy link
Owner

@rafaqz rafaqz Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be Lat/Lon/Band for GeoArray - but a GeoArray might be just a vector after indexing or something and not have them all anymore. And other formats like netcdf wont have Band - they might just be 2d lat/lon arrays.

But maybe don't worry about this for now I'll fix it up later - I'm distracting you from the main use case with the edge cases.

src/sources/gdal.jl Outdated Show resolved Hide resolved
src/sources/gdal.jl Outdated Show resolved Hide resolved
src/resample.jl Outdated Show resolved Hide resolved
src/sources/gdal.jl Outdated Show resolved Hide resolved

## Keyword Arguments
- `crs`: A `GeoFormatTypes.GeoFormat` specifying an output crs (`A` with be reprojected to `crs` in addition to being resampled). Defaults to `crs(A)`
- `method`: A `String` specifying the method to use for resampling. Defaults to `"near"` (nearest neighbor resampling). See [resampling method](https://gdal.org/programs/gdalwarp.html#cmdoption-gdalwarp-r) in the gdalwarp docs for a complete list of possible values.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaqz this link does not render when I run ?resample -- but maybe it will for the Documenter.jl-generated docs? Just wanted to flag this.

@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #73 into master will increase coverage by 2.79%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   76.70%   79.49%   +2.78%     
==========================================
  Files          16       17       +1     
  Lines         790      878      +88     
==========================================
+ Hits          606      698      +92     
+ Misses        184      180       -4     
Impacted Files Coverage Δ
src/GeoData.jl 100.00% <ø> (ø)
src/resample.jl 100.00% <100.00%> (ø)
src/sources/gdal.jl 97.18% <100.00%> (+1.99%) ⬆️
src/sources/grd.jl 95.69% <0.00%> (+0.35%) ⬆️
src/sources/ncdatasets.jl 83.85% <0.00%> (+0.74%) ⬆️
src/series.jl 87.50% <0.00%> (+0.83%) ⬆️
src/aggregate.jl 93.75% <0.00%> (+0.89%) ⬆️
src/methods.jl 83.33% <0.00%> (+0.98%) ⬆️
... and 4 more

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 5e553e1...f9730c5. Read the comment docs.

@rafaqz
Copy link
Owner

rafaqz commented Oct 13, 2020

Ok this is looking great. Just needs a few tests!

@rafaqz
Copy link
Owner

rafaqz commented Oct 13, 2020

Also I was thinking I will add resample for GeoStack and GeoSeries so you can convert whole datasets with one command.

I have an immediate use case for your snap raster idea as well - so I might have a go at adding that too. Were you thinking we could just use the "-te" and "-te-srs" flags in gdalwarp to add new extent bounds?

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 13, 2020

Were you thinking we could just use the "-te" and "-te-srs" flags in gdalwarp to add new extent bounds?

Yep, exactly! Though it might tricky if the snap raster bounds and input raster bounds don't match. Those edge cases were what prevented me from going for it right away. Though I suppose each could be padded with NoData so the extents do match? That feel a little hacky though.

For tests, I can think of a couple. 1) check that the resolution of the output matches the resolution specified in the function call and 2) check that the extent bounds are the same for the input and output GeoArrays (allowing error up to +/- the step of each dim).

@rafaqz
Copy link
Owner

rafaqz commented Oct 13, 2020

Yeah I was thinking about that too. Its easy if the input raster is larger in every bound. Padding the edge is an option. I'm not sure what GDAL will do if you specify "te" outside of the bounds of the dataset you pass it? Does it just fill with NoData anyway? (I've never actually used it manually, lol)

And those tests sound about right. Maybe we could also test something using the command line gdalwarp on a tiff to make sure they match? Not sure if that's overkill, but it would be conclusive.

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 13, 2020

Maybe we could also test something using the command line gdalwarp on a tiff to make sure they match?

I imagine ArchGDAL tests for that, so maybe testing resamples output against the results from a pure ArchGDAL workflow might effectively do that.

@rafaqz
Copy link
Owner

rafaqz commented Oct 13, 2020

Oh right that makes more sense.

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 13, 2020

Oh right that makes more sense.

Okay I'll add that test against ArchGDAL too.

I'm not sure what GDAL will do if you specify "te" outside of the bounds

I'm not 100% sure either. I remember this being an issue in GeoData when subsetting (I know that's a totally different animal though) for another workflow. I had to do something like:

left_bound = max(left_extent_raster1, left_extent_raster2)
right_bound = min(right_extent_raster1, right_extent_raster2)
lower_bound = max(lower_extent_raster1, lower_extent_raster2)
upper_extent = min(upper_extent_raster1, upper_extent_raster2)

Here's the complete code if you want to check it out. Something similar might be needed for setting -te in gdalwarp.

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 13, 2020

Also I did use gdalwarp with a snap raster in that same workflow, the extent of the snap (and also mask) raster was just always within the bounds of the input raster so I didn't have to worry about cases of non-overlapping rasters. Here's that code.

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 13, 2020

@rafaqz what are you using for data sources for testing? I'll need a .tif to test ArchGDAL output -- should I create a GeoArray, write it to test/data, then read it back in with ArchGDAL?

@rafaqz
Copy link
Owner

rafaqz commented Oct 13, 2020

I just use some random tif file from somewhere... It's in the gdal.jl test file. I need to source more and do that in an organised way.

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 14, 2020

@rafaqz tests added! I skipped checking the output extents for now (which is a test I suggested above) -- I just test for the correct output resolution and that the data (array) for the resample output matches the output from a "from scratch" resample method

@rafaqz
Copy link
Owner

rafaqz commented Oct 14, 2020

Awesome! I'll add some more when I have a go at snap. It seems the download is broken somehow, but I can merge this when that passes.

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 14, 2020

Sweet! Yeah it's something simple with download failing. Should have it up and passing soon!

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 14, 2020

🎉

@rafaqz rafaqz merged commit 7834b20 into rafaqz:master Oct 14, 2020
@rafaqz
Copy link
Owner

rafaqz commented Oct 14, 2020

croppedsnap

Looks like gdal just fills the edge :) I'll get a PR sorted tomorrow.

@vlandau
Copy link
Collaborator Author

vlandau commented Oct 14, 2020

Nice! That makes things super easy!

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.

Wrap gdal warp in a general method
3 participants