Handle `xy(NA, NA)` as null instead of empty #205

paleolimbot · 2023-10-15T17:46:57Z

@anthonynorth do you forsee problems with this? I would like to preserve the invariant that a "null feature" lines up exactly with is.na(x).

Before:

library(wk)

wk_debug(xy(NA, NA))
#> initialize (dirty = 0  -> 1)
#> vector_start: POINT[1] <0x16d26f380> => WK_CONTINUE
#>   feature_start (1): <0x16d26f380>  => WK_CONTINUE
#>     geometry_start (<none>): POINT B[EMPTY] <0x16d26f328> => WK_CONTINUE
#>     geometry_end (<none>)  => WK_CONTINUE
#>   feature_end (1): <0x16d26f380>  => WK_CONTINUE
#> vector_end: <0x16d26f380>
#> deinitialize
#> NULL
is.na(xy(NA, NA))
#> [1] TRUE

^{Created on 2023-10-15 with reprex v2.0.2}

After:

library(wk)

wk_debug(xy(NA, NA))
#> initialize (dirty = 0  -> 1)
#> vector_start: POINT[1] <0x16dc0b260> => WK_CONTINUE
#>   feature_start (1): <0x16dc0b260>  => WK_CONTINUE
#>     null_feature  => WK_CONTINUE
#>   feature_end (1): <0x16dc0b260>  => WK_CONTINUE
#> vector_end: <0x16dc0b260>
#> deinitialize
#> NULL
is.na(xy(NA, NA))
#> [1] TRUE

^{Created on 2023-10-15 with reprex v2.0.2}

codecov-commenter · 2023-10-15T17:50:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8ab32e7) 98.85% compared to head (c4753fc) 98.85%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #205   +/-   ##
=======================================
  Coverage   98.85%   98.85%           
=======================================
  Files          85       85           
  Lines        6197     6207   +10     
=======================================
+ Hits         6126     6136   +10     
  Misses         71       71

Files	Coverage Δ
R/pkg-sf.R	`99.33% <100.00%> (+0.01%)`	⬆️
R/utils.R	`100.00% <ø> (ø)`
R/vertex-filter.R	`100.00% <100.00%> (ø)`
R/xyzm.R	`100.00% <100.00%> (ø)`
src/handle-xy.c	`97.61% <100.00%> (+0.05%)`	⬆️
src/vertex-filter.c	`99.27% <ø> (-0.03%)`	⬇️
src/xy-writer.c	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anthonynorth · 2023-10-16T02:21:13Z

xy and sfc_POINT will no longer be equivalent, but that's by design. Documenting this should be enough.

Do we need an is.nan() method for xy vectors?

We'll need to check over the codebase for consistency issues with empty vs null features. I've found some bugs which are hopefully easily fixed. See reprex.

# pak::pkg_install("paleolimbot/wk#205")

# debug shows both NaN and NA are null feature?
wk::wk_debug(wk::xy(c(NA, NaN), c(NA, NaN)))
#> initialize (dirty = 0  -> 1)
#> vector_start: POINT[2] <0xffffe4850230> => WK_CONTINUE
#>   feature_start (1): <0xffffe4850230>  => WK_CONTINUE
#>     null_feature  => WK_CONTINUE
#>   feature_end (1): <0xffffe4850230>  => WK_CONTINUE
#>   feature_start (2): <0xffffe4850230>  => WK_CONTINUE
#>     null_feature  => WK_CONTINUE
#>   feature_end (2): <0xffffe4850230>  => WK_CONTINUE
#> vector_end: <0xffffe4850230>
#> deinitialize
#> NULL

# xy meta
testthat::expect_identical(
  wk::xy(NA, NA) |>
    wk::wk_meta(),
  wk::wkt(NA_character_) |>
    wk::wk_meta()
)

# xy meta
testthat::expect_identical(
  wk::xy(NaN, NaN) |>
    wk::wk_meta(),
  wk::wkt("POINT EMPTY") |>
    wk::wk_meta()
)
#> Error: wk::wk_meta(wk::xy(NaN, NaN)) (`actual`) not identical to wk::wk_meta(wk::wkt("POINT EMPTY")) (`expected`).
#> 
#> actual vs expected
#>                 geometry_type size has_z has_m precision is_empty
#> - actual[1, ]              NA   NA    NA    NA        NA       NA
#> + expected[1, ]             1    0 FALSE FALSE         0     TRUE
#> 
#>   `actual$geometry_type`: NA
#> `expected$geometry_type`:  1
#> 
#>   `actual$size`: NA
#> `expected$size`:  0
#> 
#> `actual$has_z`:   <NA> 
#> `expected$has_z`: FALSE
#> 
#> `actual$has_m`:   <NA> 
#> `expected$has_m`: FALSE
#> 
#>   `actual$precision`: NA
#> `expected$precision`:  0
#> 
#> `actual$is_empty`:   <NA>
#> `expected$is_empty`: TRUE

# xy -> sfc
testthat::expect_identical(
  wk::xy(NaN, NaN) |>
    sf::st_as_sfc(),
  wk::wkt("POINT EMPTY") |>
    sf::st_as_sfc()
)
#> Error: sf::st_as_sfc(wk::xy(NaN, NaN)) (`actual`) not identical to sf::st_as_sfc(wk::wkt("POINT EMPTY")) (`expected`).
#> 
#> `class(actual)`:   "sfc_GEOMETRYCOLLECTION" "sfc"
#> `class(expected)`: "sfc_POINT"              "sfc"
#> 
#> `actual[[1]]` is an S3 object of class <XY/GEOMETRYCOLLECTION/sfg>, a list
#> `expected[[1]]` is an S3 object of class <XY/POINT/sfg>, a double vector

# xy -> sfc
# should xy <null feature> be known to be an empty point?
testthat::expect_identical(
  # should this be point empty?
  wk::xy(NA, NA) |>
    sf::st_as_sfc(),
  wk::wkt(NA_character_) |>
    sf::st_as_sfc()
)

# round trip
testthat::expect_identical(
  wk::xy(NA, NA) |>
    sf::st_as_sfc() |>
    wk::as_xy() |>
    wk::wk_set_crs(NULL),
  wk::xy(NA, NA)
)

# impossible -> cannot succeed
testthat::expect_identical(
  wk::xy(NaN, NaN) |>
    sf::st_as_sfc() |>
    wk::as_xy() |>
    wk::wk_set_crs(NULL),
  wk::xy(NaN, NaN)
)
#> Error: wk::wk_set_crs(wk::as_xy(sf::st_as_sfc(wk::xy(NaN, NaN))), NULL) (`actual`) not identical to wk::xy(NaN, NaN) (`expected`).
#> 
#>   `actual$x`:  NA
#> `expected$x`: NaN
#> 
#>   `actual$y`:  NA
#> `expected$y`: NaN

# sfc -> xy. should this work?
testthat::expect_identical(
  sf::st_sfc(sf::st_point(c(NaN, NaN))) |>
    wk::as_xy(),
  wk::xy(NaN, NaN)
)

# coords
testthat::expect_equal(
  wk::xy(NA, NA) |>
    wk::wk_coords(),
  wk::wkt(NA_character_) |>
    wk::wk_coords()
)

# coords
testthat::expect_equal(
  wk::xy(NaN, NaN) |>
    wk::wk_coords(),
  wk::wkt("POINT EMPTY") |>
    wk::wk_coords()
)

# count
testthat::expect_equal(
  wk::wk_count(wk::xy(NA, NA)),
  wk::wk_count(wk::wkt(NA_character_))
)

# count. xy(NaN, NaN) inconsistent with other handlers
testthat::expect_equal(
  wk::wk_count(wk::xy(NaN, NaN)),
  wk::wk_count(wk::wkt("POINT EMPTY"))
)
#> Error: wk::wk_count(wk::xy(NaN, NaN)) (`actual`) not equal to wk::wk_count(wk::wkt("POINT EMPTY")) (`expected`).
#> 
#> actual vs expected
#>                 n_geom
#> - actual[1, ]        0
#> + expected[1, ]      1
#> 
#>   `actual$n_geom`: 0
#> `expected$n_geom`: 1

^{Created on 2023-10-16 with reprex v2.0.2}

anthonynorth · 2023-10-16T02:27:22Z

I think we should expect sf::st_point(c(NA, NA)) |> wk::as_xy() to be wk::xy(NaN, NaN).

# pak::pkg_install("paleolimbot/wk#205")

# sf POINT EMPTY
testthat::expect_identical(
  sf::st_sfc(sf::st_point(c(NA_real_, NA_real_))) |>
    wk::as_xy(),
  wk::xy(NaN, NaN)
)
#> Error: wk::as_xy(sf::st_sfc(sf::st_point(c(NA_real_, NA_real_)))) (`actual`) not identical to wk::xy(NaN, NaN) (`expected`).
#> 
#>   `actual$x`:  NA
#> `expected$x`: NaN
#> 
#>   `actual$y`:  NA
#> `expected$y`: NaN

# already works
testthat::expect_identical(
  sf::st_sfc(sf::st_point(c(NaN, NaN))) |>
    wk::as_xy(),
  wk::xy(NaN, NaN)
)

testthat::expect_identical(
  wk::wk_handle(
    sf::st_sfc(sf::st_point(c(NA_real_, NA_real_))),
    wk::xy_writer()
  ),
  wk::xy(NaN, NaN)
)
#> Error: wk::wk_handle(...) (`actual`) not identical to wk::xy(NaN, NaN) (`expected`).
#> 
#>   `actual$x`:  NA
#> `expected$x`: NaN
#> 
#>   `actual$y`:  NA
#> `expected$y`: NaN

testthat::expect_identical(
  wk::wk_handle(
    sf::st_sfc(sf::st_point(c(NaN, NaN))),
    wk::xy_writer()
  ),
  wk::xy(NaN, NaN)
)
#> Error: wk::wk_handle(sf::st_sfc(sf::st_point(c(NaN, NaN))), wk::xy_writer()) (`actual`) not identical to wk::xy(NaN, NaN) (`expected`).
#> 
#>   `actual$x`:  NA
#> `expected$x`: NaN
#> 
#>   `actual$y`:  NA
#> `expected$y`: NaN

^{Created on 2023-10-16 with reprex v2.0.2}

paleolimbot · 2023-10-17T01:32:26Z

Thank you for taking a look!

I think the main thing here is that xy(NaN, NaN) could be treated as EMPTY, where xy(NA, NA) could be treated as null? I'll look into the level of complexity there...it's a good point that we have that option in R.

Do we need an is.nan() method for xy vectors?

I don't think so...while a point can be missing, I'm not sure that it can be NaN in the same way that a number can be NaN.

sf::st_sfc(sf::st_point(c(NaN, NaN))) |> wk::as_xy()

I may fix that one separately...I think that happens because when all the geometries are empty, the sfc writer doesn't know what geometry to pick. In this case, we also have geometry type at the vector level and I think I can rig something to use that as a fallback.

paleolimbot · 2023-10-17T02:41:05Z

Ok, that turned into a bit of a rabbit hole, but I think it's much better. One consequence of this was that I noticed wk_coords() when there was a null POINT actually omitted a vertex (!!!!). I changed this to omit null vertices altogether...I don't remember if I had a good reason for including a null feature as a single NA coordinate.

R/vertex-filter.R

anthonynorth · 2023-10-18T01:52:11Z

Looking good.

I've noticed that sf::st_as_sfc(wk::xy(NaN, NaN)) fails. This is because the default behaviour of sf:::st_as_sf.data.frame() fails if na values are present. This isn't consistent with other formats, so perhaps it's worth patching?

# pak::pkg_install("paleolimbot/wk#205")
testthat::local_edition(3)
testthat::expect_identical(
  wk::wk_handle(
    wk::xy(NaN, NaN),
    wk::sfc_writer()
  ),
  sf::st_sfc(sf::st_point()),
  ignore_attr = c("crs", "bbox")
)

# should we default na.fail = FALSE for consistency?
testthat::expect_identical(
  wk::xy(NaN, NaN) |>
    sf::st_as_sfc(),
  sf::st_sfc(sf::st_point()),
  ignore_attr = c("crs", "bbox")
)
#> Error in st_as_sf.data.frame(as.data.frame(x), coords = xy_dims(x), crs = sf_crs_from_wk(x)): missing values in coordinates not allowed

testthat::expect_identical(
  {
    xys <- wk::xy(NaN, NaN)
    coords <- as.data.frame(xys)
    # also need to convert NaN to NA?
    coords <- as.data.frame(lapply(coords, \(x) replace(x, is.nan(x), NA_real_)))
    sf::st_as_sf(coords, coords = wk::xy_dims(xys), crs = sf::st_crs(wk::wk_crs(xys)), na.fail = FALSE) |>
      sf::st_geometry()
  },
  sf::st_sfc(sf::st_point()),
  # these appear to be inconsistencies in sf itself?
  ignore_attr = c("bbox", "n_empty")
)
#> Warning in min(cc[[1]], na.rm = TRUE): no non-missing arguments to min;
#> returning Inf
#> Warning in min(cc[[2]], na.rm = TRUE): no non-missing arguments to min;
#> returning Inf
#> Warning in max(cc[[1]], na.rm = TRUE): no non-missing arguments to max;
#> returning -Inf
#> Warning in max(cc[[2]], na.rm = TRUE): no non-missing arguments to max;
#> returning -Inf

testthat::expect_identical(
  wk::xy(NaN, NaN) |>
    wk::as_wkb() |>
    sf::st_as_sfc(),
  sf::st_sfc(sf::st_point()),
  ignore_attr = c("crs", "bbox")
)

^{Created on 2023-10-18 with reprex v2.0.2}

Edit: fix reprex

anthonynorth · 2023-10-18T01:58:09Z

I've found an inconsistency between wk_xy.sfc and using a handler.

# pak::pkg_install("paleolimbot/wk#205")
testthat::expect_identical(
  wk::as_xy(sf::st_sfc(sf::st_point())),
  wk::xy(NaN, NaN)
)
#> Error: wk::as_xy(sf::st_sfc(sf::st_point())) not identical to wk::xy(NaN, NaN).
#> Objects equal but not identical

testthat::expect_identical(
  wk::wk_handle(
    sf::st_sfc(sf::st_point()),
    wk::xy_writer()
  ),
  wk::xy(NaN, NaN)
)

^{Created on 2023-10-18 with reprex v2.0.2}

paleolimbot · 2023-10-21T01:17:45Z

Thanks again! I still have to add a few tests and fix the last issue you identified. I don't think I ever would have spotted those!

xy NA NA as empty

0ecf3ca

paleolimbot added 3 commits October 16, 2023 22:56

writer/handler consistency for xy

f24e219

with maybe working nullability/empty split in xy

89d82c8

fix the vertex filter

8d12366

add example

cadf1eb

anthonynorth reviewed Oct 18, 2023

View reviewed changes

R/vertex-filter.R Show resolved Hide resolved

fix some rough edges

7485693

paleolimbot added 3 commits October 21, 2023 21:04

stronger NA/NaN testing for sfc

1410580

fix NA/NaN for custom conversion to xy

852a5e8

fix copy/paste error

c4753fc

paleolimbot merged commit 7c0ab61 into master Oct 22, 2023
10 checks passed

paleolimbot deleted the handle-xy branch October 22, 2023 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `xy(NA, NA)` as null instead of empty #205

Handle `xy(NA, NA)` as null instead of empty #205

paleolimbot commented Oct 15, 2023

codecov-commenter commented Oct 15, 2023 •

edited

Loading

anthonynorth commented Oct 16, 2023

anthonynorth commented Oct 16, 2023 •

edited

Loading

paleolimbot commented Oct 17, 2023

paleolimbot commented Oct 17, 2023

anthonynorth commented Oct 18, 2023 •

edited

Loading

anthonynorth commented Oct 18, 2023

paleolimbot commented Oct 21, 2023

Handle xy(NA, NA) as null instead of empty #205

Handle xy(NA, NA) as null instead of empty #205

Conversation

paleolimbot commented Oct 15, 2023

codecov-commenter commented Oct 15, 2023 • edited Loading

Codecov Report

anthonynorth commented Oct 16, 2023

anthonynorth commented Oct 16, 2023 • edited Loading

paleolimbot commented Oct 17, 2023

paleolimbot commented Oct 17, 2023

anthonynorth commented Oct 18, 2023 • edited Loading

anthonynorth commented Oct 18, 2023

paleolimbot commented Oct 21, 2023

Handle `xy(NA, NA)` as null instead of empty #205

Handle `xy(NA, NA)` as null instead of empty #205

codecov-commenter commented Oct 15, 2023 •

edited

Loading

anthonynorth commented Oct 16, 2023 •

edited

Loading

anthonynorth commented Oct 18, 2023 •

edited

Loading