UrbanAnalyst / dodgr

Distances on Directed Graphs in R
https://urbananalyst.github.io/dodgr/
127 stars 16 forks source link

Suggestion: `process_bbox()` could cut the xrange between -90 and 90 (and probably the same for yrange) #156

Closed agila5 closed 3 years ago

agila5 commented 3 years ago

Hi @mpadge! I think that process_bbox() should cut the xrange between -90 and 90 (and probably the same should be applied to the yrange). For example:

dodgr:::process_bbox(c(xmin = 87.5, ymin = 22, xmax = 88.5, ymax = 23))
#> $bbox
#>        xmin   xmax
#> [1,] 90.775 91.775
#> [2,] 18.725 19.725
#> 
#> $bbox_poly
#> NULL

Created on 2021-06-04 by the reprex package (v2.0.0)

which implies that

dodgr::dodgr_streetnet(c(xmin = 87.5, ymin = 22, xmax = 88.5, ymax = 23))
#> Request failed [400]. Retrying in 1 seconds...
#> Request failed [400]. Retrying in 2.4 seconds...
#> Error in check_for_error(doc): General overpass server error; returned:
#> OSM3S ResponseThe data included in this document is from www.openstreetmap.org. The data is made available under ODbL.Error: line 3: static error: For the attribute "n" of the element "bbox-query" the only allowed values are floats between -90.0 and 90.0. Error: line 4: static error: For the attribute "n" of the element "bbox-query" the only allowed values are floats between -90.0 and 90.0. Error: line 5: static error: For the attribute "n" of the element "bbox-query" the only allowed values are floats between -90.0 and 90.0.

Created on 2021-06-04 by the reprex package (v2.0.0)

I know that I can adjust the expand parameter, but I think that the function shouldn't return an error with the default parameters.

mpadge commented 3 years ago

Ah yes indeed, great catch @agila5! Would you like to do a PR to rectify that? You'd just need to add two if statements, one for rectangular bboxes after this line and one for vector values are this line. Let me know if you'd prefer me to fix, and i'll happily do so. Thanks as always for the input!

agila5 commented 3 years ago

Just one question: what do if both xmin and xmax are > 90? Raise an error? Because I think I cannot set xmin = xmax = 90.

mpadge commented 3 years ago

Yep, i'd say an error would be in order there.

agila5 commented 3 years ago

Hi @mpadge, another related question. I don't understand the following code:

https://github.com/ATFutures/dodgr/blob/db4909638b844405136dc51b7ea13e6757f9a4aa/R/dodgr-streetnet.R#L119-L120

In particular, I think that the matrix of coordinates (i.e. the bbox object) used in those lines is specified as a matrix with 4 elements where the elements in the first row denote the xmin and xmax while the elements in the second row denote the ymin and ymax (like the output of osmdata::getbb("hampi india")). If that' the case, I think that the expand parameter should be applied as follows:

bbox [1, ] <- bbox [1, ] + c (-expand, expand) * diff (bbox [1, ])
bbox [2, ] <- bbox [2, ] + c (-expand, expand) * diff (bbox [2, ])

(i.e. by row, not by column). Does it make sense?

mpadge commented 3 years ago

But note the lines just before that: https://github.com/ATFutures/dodgr/blob/db4909638b844405136dc51b7ea13e6757f9a4aa/R/dodgr-streetnet.R#L116-L118

This is because sf uses columns on min/max and rows of x/y, whereas dodgr just tries for an implementation that is consistent between bounding polygons and bounding boxes. A polygon must have columns of x/y or lon/lat, and so dodgr insists on bounding boxes having the same. That's why the check is implemented above, to transpose any bboxes that are the other way around, notably including any sf-type bboxes. After that, the indices bbox [, i] are then appropriate. It's all a mess, because the specification of spatial bounding boxes in general terms is itself all a mess.

agila5 commented 3 years ago

Thanks for the explanation, it makes sense. IMO the only problem is that the above if clause fails when the input is specified as in the first example, i.e. dodgr:::process_bbox(c(xmin = 87.5, ymin = 22, xmax = 88.5, ymax = 23)), since (obviously) the input has no rownames or colnames. For example:

dodgr:::process_bbox(c(xmin = 87.5, ymin = 22, xmax = 88.5, ymax = 23), expand = 0.01)
#> $bbox
#>        xmin   xmax
#> [1,] 88.155 89.155
#> [2,] 21.345 22.345
#> 
#> $bbox_poly
#> NULL

Created on 2021-06-21 by the reprex package (v2.0.0)

since xmin in the "new" bbox is greater than xmin specified in the input object. I don't know if this is really a problem or just an unsupported way of specifying the input data.