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

handle various column classes #68

Merged
merged 1 commit into from
Jan 17, 2018
Merged

handle various column classes #68

merged 1 commit into from
Jan 17, 2018

Conversation

mdsumner
Copy link
Contributor

@mdsumner mdsumner commented Jan 17, 2018

This PR adds a slightly safer way of comparing column values to "{ }" and adds a curly_brace_na.data.frame method that only compares character and factor columns.

The primary motivation was to enable round-tripping with Date and POSIXct columns, which error currently (5a293b0):

library(sf)
example(st_read)
x <- st_sf(date = Sys.Date() + seq_len(2), geometry = st_geometry(nc)[1:2])
rmapshaper::ms_simplify(x)
# Error in charToDate(x) : 
 # character string is not in a standard unambiguous format

y <- st_sf(date = Sys.time() + seq_len(2), geometry = st_geometry(nc)[1:2])
rmapshaper::ms_simplify(y)
#Error in as.POSIXlt.character(x, tz, ...) : 
# character string is not in a standard unambiguous format 

I had to remove the Date metadata here in order to run ms_simplify, as a real world example:

https://github.com/mdsumner/ozplot/blame/master/README.Rmd#L100

I kind of remember discussing this at some point, so apologies if I've missed something! I think there will be other details and perhaps problematic and column types, but I feel like this is a reasonable PR without being comprehensive yet. :)

There's a further problem in that date-time types don't always(?) round-trip completely, and I assume that's a time zone thing - but it wouldn't have been noticed. I'll wait until this is accepted before pursuing those details, for now I've left out the round-trip test, commented out here:

https://github.com/ateucher/rmapshaper/pull/68/files#diff-792448277e53b51a3b944f1b1873e7f8R515

@codecov-io
Copy link

codecov-io commented Jan 17, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@5a293b0). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #68   +/-   ##
=========================================
  Coverage          ?   96.76%           
=========================================
  Files             ?       11           
  Lines             ?      587           
  Branches          ?        0           
=========================================
  Hits              ?      568           
  Misses            ?       19           
  Partials          ?        0
Impacted Files Coverage Δ
R/utils.R 94.69% <100%> (ø)

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 5a293b0...89ea2e2. Read the comment docs.

@ateucher
Copy link
Owner

Thanks @mdsumner this is great! I had actually just made an initial attempt at this in another branch, but not as comprehensive as yours: https://github.com/ateucher/rmapshaper/blob/mapshaper_v0.4.x/R/utils.R#L262-L270. The other thing I did was rather than use as.data.frame I just removed the "sf" class, because with the former it also removed tibble classes, so one would input a tibble sf object, and output a regular data.frame sf object.

Thanks so much for the tests as well. I will take this PR here and meld the best parts of our two solutions together.

Yeah, the date stuff is something I haven't tackled yet... so if you 're game, please feel free!

@ateucher ateucher merged commit 9342c88 into ateucher:master Jan 17, 2018
@ateucher
Copy link
Owner

@mdsumner FYI I merged your solution into mine, with a few tweaks, on the mapshaper_v0.4x branch here:

rmapshaper/R/utils.R

Lines 260 to 282 in b3d7c33

curly_brace_na.data.frame <- function(x) {
chr_or_factor <- vapply(x, inherits, c("character", "factor"), FUN.VALUE = logical(1))
if (any(chr_or_factor)) {
x[, chr_or_factor][x[, chr_or_factor] == "{ }"] <- NA
}
x
}
curly_brace_na.Spatial <- function(x) {
x@data <- curly_brace_na(x@data)
x
}
# This method basically just removes the sf class and then
# restores it after the data.frame method does its work, because
# the sf column is 'sticky' with `[`.sf methods, so would be
# included in the { } substitution if the sf class was kept
curly_brace_na.sf <- function(x) {
sf_col <- attr(x, "sf_column")
class(x) <- setdiff(class(x), "sf")
x <- curly_brace_na(x)
sf::st_as_sf(x, sf_column_name = sf_col)
}

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.

3 participants