Handle NA gracefully in geom_sf

[thomasp85]

Fixes #3536

This is an alternative implementation to the fix in #3537 that respects the different geometry types interpretation of NA. As the problem of NA's is unique to the vectorised constructors there is no need to handle geometrycollections in any special way as they will always be treated as scalars and grid accepts a single NA in gpar()

library(ggplot2)
library(sf)
#> Linking to GEOS 3.6.1, GDAL 2.1.3, PROJ 4.9.3

pt <- st_point(1:2)

outer <- matrix(c(0,0,10,0,10,10,0,10,0,0),ncol=2, byrow=TRUE)
pl <- st_polygon(list(outer))

# point is shown
sf1 <- st_sf(geometry = st_sfc(pl, pt))
ggplot(sf1) + geom_sf()

# point is still shown as expected
sf2 <- st_sf(geometry = st_sfc(st_geometrycollection(list(pl, pt))))
ggplot(sf2) + geom_sf()

# NA is handled fine for collections
ggplot(sf2) + geom_sf(colour = NA)

# as well as for vectorised geometries
sf3 <- st_sf(geometry = st_sfc(pt, st_point(3:4)), one_na = c(1, NA))
ggplot(sf3) + geom_sf(aes(size = one_na))

^{Created on 2019-10-01 by the reprex package (v0.2.1)}

[clauswilke]

I'm fine with this in principle. A few comments:

• Could you link to the original bug report in one of the commit messages: Regression: geom_sf() is now too aggressive in removing rows #3536

• Should we add a visual regression test for polygons with NA line color?

[clauswilke]

Is this line correct? Where does is_complete come from?

[thomasp85]

It's a function defined in utilitites.r. detect_missing() is more or less lifted from remove_missing() so that sf_grobs() can use the same logic as handle_na()

[clauswilke]

"removes rows that can't be plotted"

[clauswilke]

Could you also verify that the example from #3483 still works?

[thomasp85]

Could you link to the original bug report in one of the commit messages: #3536

It is linked at the top of the PR. Why would you want it in a commit message (that will get squashed away during merge)?

Could you also verify that the example from #3483 still works?

I've done that already (but forgot to mention). Do you want a repress of it?

[yutannihilation]

Looks good to me! Sorry for commenting this late...

[yutannihilation]

One minor problem of this approach to move the process to outside of handle_na() is that na.rm no longer controls whether or not to warn; we need to choose always be quiet or always warn. I feel the latter is slightly better, but am not fully sure what should be done here.

> test_check("ggplot2")

── 1. Failure: geom_sf() removes rows containing missing aes (@test-geom-sf.R#85

`expect_identical(...)` did not produce any warnings.

── 2. Failure: geom_sf() removes rows containing missing aes (@test-geom-sf.R#89

`expect_identical(...)` did not produce any warnings.

── 3. Failure: geom_sf() removes rows containing missing aes (@test-geom-sf.R#94

`expect_identical(...)` did not produce any warnings.

(https://travis-ci.org/tidyverse/ggplot2/jobs/591938837#L1219-L1227)

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/