fill() not replacing NAs when value type is period

[joeycouse]

I've noticed fill() does not support value type period. I didn't see anything in the documentation mentioning this case or any warnings that certain values couldn't be filled.

library(tidyverse)
library(lubridate)
#> 
#> Attaching package: 'lubridate'
#> The following objects are masked from 'package:base':
#> 
#>     date, intersect, setdiff, union

set.seed(24)

n <- 2

df <- tibble(
  my_type = sample('a', n, replace = T),
  time = period(12, units = 'second'),
  value = sample(c(NA, 5), n, replace = F)
)

df[2,2] = NA

df
#> # A tibble: 2 x 3
#>   my_type time     value
#>   <chr>   <Period> <dbl>
#> 1 a       12S          5
#> 2 a       NA          NA

df %>%
  group_by(my_type) %>%
  fill(everything(), .direction = 'down')
#> # A tibble: 2 x 3
#> # Groups:   my_type [1]
#>   my_type time     value
#>   <chr>   <Period> <dbl>
#> 1 a       12S          5
#> 2 a       NA           5

^{Created on 2021-02-18 by the reprex package (v1.0.0)}

[DavisVaughan]

Did you mean to group_by(my_type)? fill() respects groups, so the NA won't get filled in with 12S because it is in a separate group.

library(tidyr)
library(lubridate)

x <- seconds(c(1, NA, 2, NA, 1))
df <- data.frame(x = x)

fill(df, x, .direction = "down")
#>    x
#> 1 1S
#> 2 1S
#> 3 2S
#> 4 2S
#> 5 1S

^{Created on 2021-02-20 by the reprex package (v1.0.0)}

[joeycouse]

The group_by(my_type) wasn't necessary since there is a single value of my_type , but the result is still the same.

library(tidyverse)
library(lubridate)
#> 
#> Attaching package: 'lubridate'
#> The following objects are masked from 'package:base':
#> 
#>     date, intersect, setdiff, union

set.seed(24)

n <- 2

df <- tibble(
  my_type = sample('a', n, replace = T),
  time = period(12, units = 'second'),
  value = sample(c(NA, 5), n, replace = F)
)

df[2,2] = NA

df
#> # A tibble: 2 x 3
#>   my_type time     value
#>   <chr>   <Period> <dbl>
#> 1 a       12S          5
#> 2 a       NA          NA

df %>%
  fill(everything(), .direction = 'down')
#> # A tibble: 2 x 3
#>   my_type time     value
#>   <chr>   <Period> <dbl>
#> 1 a       12S          5
#> 2 a       NA           5

^{Created on 2021-02-20 by the reprex package (v1.0.0)}

[DavisVaughan]

Ah I see. tidyr:::fillDown() is only filling the first slot of the Period S4 object. Switching to vec_fill_missing() internally would probably solve this.

library(lubridate)
library(vctrs)
library(tidyr)

x <- seconds(c(1, NA))

str(x)
#> Formal class 'Period' [package "lubridate"] with 6 slots
#>   ..@ .Data : num [1:2] 1 NA
#>   ..@ year  : num [1:2] 0 NA
#>   ..@ month : num [1:2] 0 NA
#>   ..@ day   : num [1:2] 0 NA
#>   ..@ hour  : num [1:2] 0 NA
#>   ..@ minute: num [1:2] 0 NA

# looks good?
x_fill <- tidyr:::fillDown(x)
x_fill
#> [1] "1S" "1S"

# nope, not good
str(x_fill)
#> Formal class 'Period' [package "lubridate"] with 6 slots
#>   ..@ .Data : num [1:2] 1 1
#>   ..@ year  : num [1:2] 0 NA
#>   ..@ month : num [1:2] 0 NA
#>   ..@ day   : num [1:2] 0 NA
#>   ..@ hour  : num [1:2] 0 NA
#>   ..@ minute: num [1:2] 0 NA

# tibble/pillar sees this as an NA for whatever reason
tibble::tibble(x = x_fill)
#> # A tibble: 2 x 1
#>   x       
#>   <Period>
#> 1 1S      
#> 2 NA


x_fill_vctrs <- vec_fill_missing(x, direction = "down")

# better
str(x_fill_vctrs)
#> Formal class 'Period' [package "lubridate"] with 6 slots
#>   ..@ .Data : num [1:2] 1 1
#>   ..@ year  : num [1:2] 0 0
#>   ..@ month : num [1:2] 0 0
#>   ..@ day   : num [1:2] 0 0
#>   ..@ hour  : num [1:2] 0 0
#>   ..@ minute: num [1:2] 0 0

tibble::tibble(x = x_fill_vctrs)
#> # A tibble: 2 x 1
#>   x       
#>   <Period>
#> 1 1S      
#> 2 1S

^{Created on 2021-02-20 by the reprex package (v1.0.0)}