rename / refactor / deprecate scan_to_spdf

[adokter]

1. @bart1 Do we really need function scan_to_spdf? It's a helper function that is used in integrate_to_ppi for projecting a scan onto a pre-defined grid. Why can't we use the existing scan_to_raster() function for that?

2. note that rasterToPoints() used in the statement where you call scan_to_spdf throws out any raster points with NA values, is that the desired behaviour? If so, please explain in documentation.

3. Could you add a user example to the roxygen documentation, preferably using bioRad's native example_scan object or the example_pvol object that can be loaded as follows:

pvolfile <- system.file("extdata", "volume.h5", package = "bioRad")
# load the file:
example_pvol <- read_pvolfile(pvolfile)

4. Instead of recycling the res argument for parsing user-definable rasters, I think it's better to introduce a new input parameter for this, and clarify in the documentation that when specified it overrides res and other arguments.

5. any requirements for the user-definable raster? like CRS requirements?

[bart1]

Should we use raster as an argument for the target projection argument?

[bart1]

I can see the point of not having scan_to_spdf the reason to keep it would be a speed reason. integrate_to_ppi now calls scan_to_raster for each scan separately then within scan_to_raster spTransform is called. This means spTransform is called for each scan separately, When using scan_to_spdf integrate_to_ppi can perform one conversion of the grid to a local flat projection and then avoid other transformations. This means for a pvol with 13 scans, 12 calls to spTransform are avoided. The general speed up is 2 to 3 times, see some of the examples below:

setwd('~/bioRad')
devtools::load_all()
#> Loading bioRad
#> Welcome to bioRad version 0.5.0.9289
#> Docker daemon running, Docker functionality enabled (vol2bird version 0.5.0)
pvolfile <- system.file("extdata", "volume.h5", package = "bioRad")
# load polar volume
example_pvol <- read_pvolfile(pvolfile)
# load the corresponding vertical profile for this polar volume
data(example_vp)
# calculate the range-corrected ppi on a 50x50 pixel raster
# when working with rasters the following approach can be used:
template_raster<-raster::raster(raster::extent(12,13,56,57), crs=sp::CRS('+proj=longlat'))

microbenchmark::microbenchmark(times=5,
my_ppi<-integrate_to_ppi(example_pvol, example_vp, res=template_raster) ,
my_ppi<-integrate_to_ppi_spdf(example_pvol, example_vp, res=template_raster) 
)
#> Unit: seconds
#>                                                                              expr
#>       my_ppi <- integrate_to_ppi(example_pvol, example_vp, res = template_raster)
#>  my_ppi <- integrate_to_ppi_spdf(example_pvol, example_vp, res = template_raster)
#>       min       lq     mean   median       uq       max neval cld
#>  2.586905 2.600590 4.603803 3.418319 3.950116 10.463086     5   a
#>  1.260590 1.282675 2.162907 2.179974 2.685364  3.405934     5   a


template_raster<-raster::raster(raster::extent(12,13,56,57), crs=sp::CRS('+proj=longlat'), res=.01)

microbenchmark::microbenchmark(times=5,
                               my_ppi<-integrate_to_ppi(example_pvol, example_vp, res=template_raster) ,
                               my_ppi<-integrate_to_ppi_spdf(example_pvol, example_vp, res=template_raster) 
)
#> Unit: seconds
#>                                                                              expr
#>       my_ppi <- integrate_to_ppi(example_pvol, example_vp, res = template_raster)
#>  my_ppi <- integrate_to_ppi_spdf(example_pvol, example_vp, res = template_raster)
#>       min       lq     mean   median       uq      max neval cld
#>  3.090922 3.152694 3.485145 3.160893 3.217325 4.803889     5   b
#>  1.322290 1.392180 1.412335 1.413996 1.415329 1.517883     5  a

require(uvaRadar)
#> Loading required package: uvaRadar
example_pvol<-get_pvol('NL/HRW')
example_vp<-get_vp('NL/HRW')
template_raster<-raster::raster(raster::extent(5,6,51,52), crs=sp::CRS('+proj=longlat'), res=.01)
microbenchmark::microbenchmark(times=5,
                               my_ppi<-integrate_to_ppi(example_pvol, example_vp, res=template_raster) ,
                               my_ppi<-integrate_to_ppi_spdf(example_pvol, example_vp, res=template_raster) 
)
#> Warning in integrate_to_ppi(example_pvol, example_vp, res = template_raster):
#> ignoring 90 degree birdbath scan

#> Warning in integrate_to_ppi(example_pvol, example_vp, res = template_raster):
#> ignoring 90 degree birdbath scan

#> Warning in integrate_to_ppi(example_pvol, example_vp, res = template_raster):
#> ignoring 90 degree birdbath scan

#> Warning in integrate_to_ppi(example_pvol, example_vp, res = template_raster):
#> ignoring 90 degree birdbath scan
#> Warning in integrate_to_ppi_spdf(example_pvol, example_vp, res =
#> template_raster): ignoring 90 degree birdbath scan

#> Warning in integrate_to_ppi_spdf(example_pvol, example_vp, res =
#> template_raster): ignoring 90 degree birdbath scan

#> Warning in integrate_to_ppi_spdf(example_pvol, example_vp, res =
#> template_raster): ignoring 90 degree birdbath scan

#> Warning in integrate_to_ppi_spdf(example_pvol, example_vp, res =
#> template_raster): ignoring 90 degree birdbath scan

#> Warning in integrate_to_ppi_spdf(example_pvol, example_vp, res =
#> template_raster): ignoring 90 degree birdbath scan
#> Warning in integrate_to_ppi(example_pvol, example_vp, res = template_raster):
#> ignoring 90 degree birdbath scan
#> Unit: seconds
#>                                                                              expr
#>       my_ppi <- integrate_to_ppi(example_pvol, example_vp, res = template_raster)
#>  my_ppi <- integrate_to_ppi_spdf(example_pvol, example_vp, res = template_raster)
#>        min        lq      mean    median        uq       max neval cld
#>  15.195955 15.855693 16.328769 16.644581 16.668140 17.279474     5   b
#>   4.984612  5.084243  5.792908  5.967457  6.027665  6.900564     5  a

^{Created on 2020-02-05 by the reprex package (v0.3.0)}

[adokter]

Should we use raster as an argument for the target projection argument?

sounds good

[adokter]

This means spTransform is called for each scan separately, when using scan_to_spdf integrate_to_ppi can perform one conversion of the grid to a local flat projection and then avoid other transformations. This means for a pvol with 13 scans, 12 calls to spTransform are avoided.

Hi @bart1, but scan_to_spdf also operates on each scan separately, however without calling spTransform:

bioRad/R/integrate_to_ppi.R

Lines 216 to 220 in 9d89575

	rasters <- lapply(pvol$scans, function(x) {
	scan_to_spdf(
	add_expected_eta_to_scan(x, vp, param = param, lat = lat, lon = lon, antenna = antenna, beam_angle = beam_angle, k = k, re = re, rp = rp),
	spdf=spdf, param = c("range", "distance", "eta", "eta_expected"), k = k, re = re, rp = rp)
	})

The main difference difference with scan_to_raster is that in scan_to_spdf you avoid the use of spTransform by:

1. converting your input grid to aeqd projection centered at the radar location (using spTransform once)
2. use faster native bioRad functions (cartesian_to_polar and polar_to_index) to project each scan to aeqd projection (without using spTransform)
3. map the aeqd projected data back to the user-defined input raster.

A solution may be to refactor scan_to_spdf to a function pvol_to_raster, which projects a full polar volume on a raster(brick). This function could speed up the projection of multiple scans on the same grid, similar to your solution in scan_to_spdf (using less spTransform). That function could also be exposed to user space

[adokter]

makes me wonder if spTransform in sp package is fairly slow relative to the newer sf package, to which we will have to migrate at some point (see #214).

[bart1]

The pvol to raster function makes sense, I have been thinking of doing something like that but then to stars, the reason being that raster bricks do not have a very satisfactory way to keep track of the attributes of each parameter/scan while in stars one would be able to nicely separate between the elevation scans as a extra dimension and the parameters as separate parameters. But I feel adding stars might be outside the scope of 0.5.0. How should a raster brick containing a full polar volume look like?
The closest I can think of is something like this:

require(raster)
#> Loading required package: raster
#> Loading required package: sp
b <- brick(system.file("external/rlogo.grd", package="raster"))
names(b)<-c('DBZH', 'TH', 'VRADH')
b<-brick(list(b,b))
b<-setZ(b,rep(c(.5,.8), each=3),'elevationAngle')
b
#> class      : RasterBrick 
#> dimensions : 77, 101, 7777, 6  (nrow, ncol, ncell, nlayers)
#> resolution : 1, 1  (x, y)
#> extent     : 0, 101, 0, 77  (xmin, xmax, ymin, ymax)
#> crs        : +proj=merc +datum=WGS84 +ellps=WGS84 +towgs84=0,0,0 
#> source     : memory
#> names      : DBZH.1, TH.1, VRADH.1, DBZH.2, TH.2, VRADH.2 
#> min values :      0,    0,       0,      0,    0,       0 
#> max values :    255,  255,     255,    255,  255,     255 
#> elevationAngle: 0.5, 0.5, 0.5, 0.8, 0.8, 0.8

But I find the usage of names like DBZH.1 not really acceptable.

Maybe we can make scan_to_raster not use spTransform when the right projection (aeqd) is provided?

[bart1]

I avoided changes to scan_to_raster to avoid unforeseen consequences but thinking about it that might be best on this time scale

[adokter]

Maybe we can make scan_to_raster not use spTransform when the right projection (aeqd) is provided?

That would be a nice solution

[adokter]

But I find the usage of names like DBZH.1 not really acceptable.

True, so maybe wait for a moment when we feel like porting to stars and sf. A list of rasterbricks might be an option as well for pvol_to_raster output, but also ugly ...

[bart1]

Maybe we can make scan_to_raster not use spTransform when the right projection (aeqd) is provided?

That would be a nice solution

I now kind of realize what makes this a bit more involved, since after re projecting the raster is not a grid anymore (X and Y distances are not equal/uniform) it would be required for scan_to_raster to accept some form of spatial points object. I will give that a try.

[adokter]

right ... but parsing a list of points to the scan_to_raster function is very confusing for users as well.

The use case for scan_to_spdf is essentially a speed up when projecting multiple scans on the same user grid, and avoid duplicate transformations of the same user grid to aeqd projection multiple times.

That makes me think we have to write a scans_to_raster function (that can take as input a list of scans, like in a polar volume), which outputs a list of rasters. The user function scan_to_raster will then be a special case of that more generic scans_to_raster function (which will already be close to a pvol_to_raster function). Let me know what you think

[bart1]

I think we ultimately need a pvol_to_raster function (possibly even pvol_to_spatial, I think at some stage one would like to project to maybe non regular points). A list of scans to a list of raster bricks might be a quick proxy.
An alternative I could see that might be a quick proxy is to make scan_to_raster use scan_to_spdf and keep that not exported for the time being. In this case integrate_to_ppi would also use this later function. This would at least avoid code duplication.
Probably scans_to_raster is the best that can be done without adding a dependency on stars

[adokter]

Let's keep scan_to_spdf for now and bump this issue to the next release

[bart1]

I think this is better solved when moving to sf see #214