satellogic / telluric

telluric is a Python library to manage vector and raster geospatial data in an interactive and easy way
MIT License
87 stars 18 forks source link

Add warp_mem_limit and num_threads to rasterio reproject calls #285

Open eecarres opened 3 years ago

eecarres commented 3 years ago

This commit aims to expose the two available parameters in rasterio reproject method, used to control its performance

codecov-io commented 3 years ago

Codecov Report

Merging #285 (b44f998) into master (a1529cc) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #285   +/-   ##
=======================================
  Coverage   90.64%   90.64%           
=======================================
  Files          37       37           
  Lines        5943     5943           
=======================================
  Hits         5387     5387           
  Misses        556      556           
Impacted Files Coverage Δ
telluric/georaster.py 93.51% <100.00%> (ø)

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 a1529cc...b44f998. Read the comment docs.

arielze commented 3 years ago

@eecarres this looks like a good way to go, although I recommend to introduce a more generic approach, something like, rasterio_kwargs which accepts a dictionary of arguments passed to rasterio.warp.reproject

eecarres commented 3 years ago

@arielze Whatever you find the most convenient. What I saw in the telluric code was that direct calls to rasterio reproject were done with the option of giving all the parameters explicitly, that's why I tried to "continue with the trend". As you can see here the rasterio call also has a kwargs, but the rest of the parameters (except from the two I added) are explicitly defined in the telluric calls.

As I said, you decide =D

drnextgis commented 3 years ago

I do agree with @arielze. These two parameters are too low level and we don't deal with them in the source code. So I think it worth to create a new **kwargs argument for merge_all. In this case we don't need to copypaste docstring with description of these parameters into myriads of internal method and also it allows us to use new arguments that might be added in the future to rasterio.