A few initial thoughts after trying the package (only for 10mins or so!)

[everydayduffy]

This is a really cool resource Hugh, and I can imagine it being useful for both teaching and research. A few very minor thoughts/suggestions for you. Feel free to ignore!

1) Given that you are using underscores in your function names, I think it is more consistent to use them in the functions arguments. A mix and match of . and _ is a bit stylistically inconsistent. 2) When requesting a resolution that is too small/unavailable, I got a bit of a barrage of error messages, some which repeated:

trying URL 'https://environment.data.gov.uk/UserDownloads/interactive/3af527a994b442a49d17cb029f5dd706121700/LIDARCOMP/LIDAR-DTM-25CM-2019-SW73ne.zip'

Requested tile is not available!!!

        Either: (1) No data is availale in this tile or 

        (2) try a different resolution... 

trying URL 'https://environment.data.gov.uk/UserDownloads/interactive/3af527a994b442a49d17cb029f5dd706121700/LIDARCOMP/LIDAR-DTM-25CM-2019-SW73se.zip'

Requested tile is not available!!!

        Either: (1) No data is availale in this tile or 

        (2) try a different resolution... 

Error in raster::crop(ras.merge, sf_geom) : object 'ras.merge' not found

I wonder if the stop/message could come a bit earlier to nip it in the bud and stop trying twice?

3) A potential feature could be that when a polygon in a non-BNG projection is provided, to return a raster in the projection of the polygon? Also, when providing a polygon that's not in BNG, a message that it is being reprojected might be useful, just to let the user know what's going on behind the scenes.

All very minor things though. Great work!

[h-a-graham]

Thanks James! Really helpful comments and totally agree with all of them. Especially point 1 given the style of arguments - whilst flicking between python and R styles I kind of lost myself - I'll go with underscores for everying and keep it consistent!

On point 3 this is a nice idea - certainly adding a warning re. the transformation is important. Do you think it is safe to presume that the user wants the output raster in the same format as the input polygon? If so, is this the kind of thing you'd like a warning for or would you consider it acceptable default behaviour?

So for point 2 - I agree this can get a bit much. The issue is that I'm actually not sure of the exact 5km tiles that do or don't store data... Perhaps a better and more useful warning message could be given on completion specifying the tiles that don't contain data?

Thanks again!

[everydayduffy]

Hi Hugh,

Regarding point 3 - I would say default BNG output raster since this is the native format of the data. Any transformation/warping of those data should come with a message/warning. So if the user gives a BNG sf object and the function spits out a BNG raster = no warning. WGS input sf -> output BNG raster = message/warning. You could add an optional argument to the function to force the output to same projection of input if the input projection isn't BNG. Needs a bit of thought for different scenarios though. I think 99% of your users will be happy with a BNG output so maybe don't both adding too many bells and whistles in that regard!

Point 2 - hmm I wonder if there's a way to produce a lookup table to query before trying?

Oh and one other thing - do you know if there is a way to disable the popup download progress bar? I have a feeling this "steals" the focus, and therefore you can't use your cursor easily to carry on typing things into the console. I've had it with another package where I was downloading data for about 10 mins and couldn't get anything else done in that RStudio window!

[h-a-graham]

Cheers James,

Yeah good points. So i had a play with the option to reproject the rasters this morning and came up against an issue. This would only be sensible when merge has been selected - if choosing to save individual tiles (which is most likely desired for donwloading large areas) reprojecting individual tiles makes subsequent merging very risky I think. So I think best to go with your idea of warning but (at least for now) always returning rasters in BNG.

Yes a lookup table could work - would need to create these first for all available datasets with the EA's coverage vectors - not too problematic... However, it is perhaps not necessary because the time penalty for the current approach is next to nothing. I posted another issue for a new feature where the user can check coverage before downloading which might help to some degree. Let me think about this - I'll create a new issue on this once I've solved the others that you've raised.

Which reminds me - at present there is no error message if no data exists for the whole requested area so that should definitely be implented.

Agree with you on the download progress bars - I also don't find the print messages helpful either. Ideally I'd like to silence all of this and maybe replace with a simple progress bar in get_area. However, I've used purrr which doesn't have a built in progress bar so maybe something like this would be good: https://www.r-bloggers.com/2019/01/purrring-progress-bars-adding-a-progress-bar-to-purrrmap/

Thanks again for your comments - it's really appreciated!

[h-a-graham]

Cheers Again for these @everydayduffy . I'll close this issue for now but will reopen one re. the join tables when I find time. Let me know if you have any more thoughts/suggestions!