Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement osmdata_data_frame supporting out meta; and adiff queries #285

Merged
merged 18 commits into from
Dec 13, 2022

Conversation

jmaspons
Copy link
Collaborator

Works and seems that doesn't break anything (check is fine). I will add tests if you like the idea.

@jmaspons jmaspons marked this pull request as draft December 1, 2022 10:32
@mpadge
Copy link
Member

mpadge commented Dec 1, 2022

See also #252, which should be retained as the "original" vision of this kind of function, courtesy of @Mashin6. No XML filtering; no nodes, ways, rels; just data. So any osmdata_data_frame() function should first of all do that, and then subsequently be overloaded for the kinds of queries envisioned here if desired/necessary. That original vision was to provide access to overpass queries which are intended to return tabular data, and tabular data only.

@jmaspons Would you be able to modify this to reflect #252, and return that kind of data.frame, and yet still incorporate the functionality suggested here?

Old fill_overpass_data return either the doc as an XML object or as a
character vector, depending on the missing or not doc parameter.

All the osmdata_* functions cast the doc to character with:
    rcpp_osmdata_* (paste0 (doc))
and therefore are not affected by this change.
@jmaspons
Copy link
Collaborator Author

jmaspons commented Dec 1, 2022

See also #252, which should be retained as the "original" vision of this kind of function, courtesy of @Mashin6. No XML filtering; no nodes, ways, rels; just data. So any osmdata_data_frame() function should first of all do that, and then subsequently be overloaded for the kinds of queries envisioned here if desired/necessary. That original vision was to provide access to overpass queries which are intended to return tabular data, and tabular data only.

I think the proposal is unrelated as it affects mostly opq and how to allow the user to change prefix and suffix, which I am also interested in for other use cases. Let's talk about it there.

@jmaspons Would you be able to modify this to reflect #252, and return that kind of data.frame, and yet still incorporate the functionality suggested here?

Once #252 is implemented, it should be easy to handle the case by parsing the opq$suffix or whatever flag that the implementation end up with, but I think it's premature while there are no option for different prefixes and suffixes. I will help to modify osmdata_data_frame when needed.

P.S.: reading your post, I interpret that you prefer osmdata_data_frame() as a function name. I'll do it

@jmaspons jmaspons marked this pull request as ready for review December 1, 2022 11:52
jmaspons and others added 2 commits December 2, 2022 14:32
Profiling shows that list2DF is much faster than data.frame(list(...))
@jmaspons jmaspons changed the title Implement osmdata_data.frame Implement osmdata_data_frame supporting out meta; and adiff queries Dec 4, 2022
Fixes to get_metadata to guess query type for empty results
@jmaspons
Copy link
Collaborator Author

jmaspons commented Dec 4, 2022

osmdata_data_frame works with adiff queries, partially solving #179

@mpadge
Copy link
Member

mpadge commented Dec 6, 2022

@jmaspons I'll get to this one asap, but may take a couple of days.

Still much slower than osmdata_sf
adiff have <new> node for deleted objects, but diff have not.
diff results may contain duplicated entries -> unique(df)
@mpadge mpadge mentioned this pull request Dec 7, 2022
6 tasks
R/get-osmdata.R Show resolved Hide resolved
Copy link
Member

@mpadge mpadge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from that one comment, this looks great!

The inputs should come from an opq object, thus, the datetimes already have passed check_datetime()
@mpadge
Copy link
Member

mpadge commented Dec 13, 2022

@jmaspons This seems good to merge now. Did you have any other changes in mind? If not, then I'll merge straight away. I had a play around with it - this really is a very nice addition! I think i'll end up using this quite a bit.

@jmaspons
Copy link
Collaborator Author

Great! Go ahead with the merge. I will rebase the other PR on top of this one. Do you have any preference for the order of PR from #292 ? If not, I will go for #288

@mpadge mpadge merged commit 9f2fd6c into ropensci:main Dec 13, 2022
@jmaspons jmaspons deleted the osmdata_data.frame branch February 2, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants