Modifications to osm_line2poly

[richardbeare]

I hope these changed deal with many strange scenarios I encountered, such as broken and looping ways. The function now joins ways together, creates polygons with left over ways. I've also reorganised some of the logic to deal with, for example, coastline entering and leaving the same side of the bbox.

OSM ways have the land on the left, so I'm confident that we can reliably distinguish sea and ocean polygons, but I'm less confident that the procedure I've implemented is foolproof. It is likely to do funny things to transparent layers, and an "prison bars" style landforms are probably going to cause issues.

The trickiness of the geometry suggests that there is merit in good interfaces to the sp/sf/spatstat family of packages, at least for testing solutions to this kind of problems. For example there is st_line_merge in sf which solves part of the problem I had.

I've returned lines in single structures (via rbind). I've noticed that recent comments suggested going with lists. Anyhow - see what you think....

Vignette added which contains one big example and the test cases, as I'm not sure how to put them into useful testthat format.

[mpadge]

Richard, this is awesome stuff! Thanks so much for doing a way better job than my quick hack. Can you please just fix up the travis issues with the vignette (lines 79-81)? This is the first call to osm_line2poly(). (Here, for example.) Once that's passing, I'll PR and put you on the "aut" list for the package with these mighty contributions.

[codecov-io]

Codecov Report

Merging #27 into master will increase coverage by 1.54%. The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
+ Coverage   85.56%   87.11%   +1.54%     
==========================================
  Files          21       21              
  Lines        2418     2375      -43     
==========================================
  Hits         2069     2069              
+ Misses        349      306      -43

Impacted Files	Coverage Δ
R/line2poly.R	0% <0%> (ø)	:arrow_up:

---

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 00d4ebb...566a966. Read the comment docs.

[richardbeare]

I'm having a lot of trouble getting this to work properly on the various CI machines. The issue always stems from partial failures of osm queries, leading to incomplete map data. One solution is to save the data from a successful query, but the main example has quite a large data file (48M). I can test for successful queries at various points, but still end up with issues around png files used in the Rmd file. The last point wouldn't be an issue if the figures were included in the normal way. Where to from here?

[mpadge]

Oh, I understand. The overpass servers aren't reliable enough to be used for tests, either on CRAN or travis. Most of the package is tested with fake data, with full tests only run locally. The vignettes can't really use fake data, yet they also can't use actual overpass calls because they have to compile to pass. I'd suggest just including pre-generated figures and switching all code off (eval = FASE), as is done extensively throughout both of the current vignettes. Could you just do that please, and then I'll try to write tests using fake data.

Note that this will likely require some reasonably small sample, so if you could find that, rather than the entirety of Melbourne, that'd be great. Just pick some place in the world with a contorted coastline and an island that is as small as possible? In current form, it likely won't be possible to hide the fake data in the package because it's way too large. (Although it may nevertheless be possible by stripping away everything except what is necessary for the figs, partiuclarly almost all of the sf non-geometry data objects. These matrices generally only need the osm_id and geometry columns to print, so can be massively compressed that way. Don't stress too much if you can't find alternative locations.)

---

Separate point, because i didn't actually ask: Are you cool with being added as "aut"? Other option would be "ctb" (contributor), but since you went and wrote and entire vignette, I'd say that really is an authorial contribution, and I'd be totally happy to list you as package co-author., a gesture also reflective of my appreciation of your efforts here! Cool with that?

[richardbeare]

Sounds good - I'll commit the pngs etc. Definitely appreciate authorship - wasn't anticipating that.

Most of the tests at the end of the vignette are small, so they won't need much change. Probably can't test the combination of ways and topology without a large sample. But a few more small pieces will cover most of the problem cases.

On Mon, Sep 25, 2017 at 4:37 AM, mark padgham notifications@github.com wrote:

Oh, I understand. The overpass servers aren't reliable enough to be used for tests, either on CRAN or travis. Most of the package is tested with fake data, with full tests only run locally. The vignettes can't really use fake data, yet they also can't use actual overpass calls because they have to compile to pass. I'd suggest just including pre-generated figures and switching all code off (eval = FASE), as is done extensively throughout both of the current vignettes. Could you just do that please, and then I'll try to write tests using fake data.

Note that this will likely require some reasonably small sample, so if you could find that, rather than the entirety of Melbourne, that'd be great. Just pick some place in the world with a contorted coastline and an island that is as small as possible? In current form, it likely won't be possible to hide the fake data in the package because it's way too large. (Although it may nevertheless be possible by stripping away everything except what is necessary for the figs, partiuclarly almost all of the sf non-geometry data objects. These matrices generally only need the osm_id and geometry columns to print, so can be massively compressed that way. Don't stress too much if you can't find alternative locations.)

Separate point, because i didn't actually ask: Are you cool with being added as "aut"? Other option would be "ctb" (contributor), but since you went and wrote and entire vignette, I'd say that really is an authorial contribution, and I'd be totally happy to list you as package co-author., a gesture also reflective of my appreciation of your efforts here! Cool with that?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ropensci/osmplotr/pull/27#issuecomment-331729974, or mute the thread https://github.com/notifications/unsubscribe-auth/AAvoobzVwmpR5rMe8uo4u511toPyOneCks5slqFjgaJpZM4PfJwZ .