Add lat and lon columns for out * center; queries in osmdata_data_frame
#316
Conversation
When results only contain nodes, there is no way to distinguish if center lat & long columns have to be included or not
have 'tags center' return NA values from C++ code instead of DOUBLE_MAX
|
Thanks for the review, @mpadge. Everything addressed, so I'm going to merge |
There was a problem hiding this comment.
I think that should be okay once those changes have been implemented, but maybe just check downstream in the general workflow here. One thing i'm not sure about - due to me not understanding it fully 😏 - is these lines (in "get-osmdata-df.R"::xml_to_df()):
if (any (missing_center)) { # not a "out * center;" query
center <- lapply (center, function (x) {
x[, c ("osm_center_lat", "osm_center_lon")] <- NULL
x
})
}They now don't do anything at all, right? And so can be removed, right?
| this <- this [, which (has_data), drop = FALSE] | ||
| if (ncol (this) > 0L) { | ||
| colnames (this) <- paste0 ("osm_center", colnames (this)) | ||
| } |
There was a problem hiding this comment.
| } | |
| } else { | |
| this <- this [-seq_len (nrow (this)), ] | |
| } |
There was a problem hiding this comment.
That's necessary because of the rownames. Having those means causes rows to be retained even when all columns are removed, so nrow(this) remains at whatever value it was, and not zero. That line removes all rows any time that the columns are removed, ensuring a null data.frame is returned.
There was a problem hiding this comment.
But we need that nrows match other data.frames, so a nrows x 0 dimensions data.frame is fine and necessary, otherwise the code will fail around
Line 126 in 31b9a6e
And all the tests pass so I think it's fine as it is
Co-authored-by: mark padgham <mark.padgham@email.com>
This part remove osm_center_* columns if in Removing the columns like this instead of |
For calls to
osmdata_data_framethat do not include the query and the results only contain nodes, there is no way to distinguish if center lat & lon columns have to be added or not. The current implementation adds osm_center_lat/lon in these casesFix #315