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

Preserve classes when reading from validation_set$values #539

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

yjunechoe
Copy link
Collaborator

@yjunechoe yjunechoe commented Jun 13, 2024

Summary

This PR preserves the class of value where possible. Two consequences:

  1. When value and column are not of the same class (and thus their interaction is not well defined), the associated warning/error now gets to be produced at interrogate(), which shows up at interrogate/report. Using a modified reprex from Evaluation of datetimes not as expected  #536:
# NOTE: {lubridate} not loaded for this chunk
devtools::load_all()

df <- data.frame(
  date = Sys.Date() + 1,
  datetime = Sys.time() + 1
)

agent_base_loaded <- create_agent(df) |> 
  col_vals_lte(datetime, Sys.Date()) |> 
  col_vals_lte(date, Sys.Date()) |>
  interrogate(show_step_label = TRUE)
#> 
#> ── Interrogation Started - there are 2 steps ──────────────────────────────────
#> ℹ Step 1: an evaluation issue requires attention (WARNING).
#> ✔ Step 2: OK.
#> 
#> ── Interrogation Completed ────────────────────────────────────────────────────

agent_base_loaded$validation_set$capture_stack[[1]]$warning
#> <warning/rlang_warning>
#> Warning:
#> There was 1 warning in `dplyr::mutate()`.
#> ℹ In argument: `pb_is_good_ = datetime <= structure(19887, class = "Date")`.
#> Caused by warning:
#> ! Incompatible methods ("Ops.POSIXt", "Ops.Date") for "<="
  1. When methods are defined for the class, they get used by pointblank at interrogate(). So if we define the same agent after {lubridate} has been loaded, we get the more sensible behavior for comparing datetime to date:
library(lubridate)

# Loading lubridate puts the custom method `<=.POSIXt` on the search path,
# which overrides the problematic `Ops.POSIXt` method from base
sloop::s3_dispatch(df$datetime <= Sys.Date())
#>    <=.POSIXct
#> => <=.POSIXt
#>    <=.default
#>    Ops.POSIXct
#> -> Ops.POSIXt
#>    Ops.default
#> -> <= (internal)

# This is picked up on evaluation - datetime can now be compared with date without warning
agent_lubridate_loaded <- create_agent(df) |> 
  col_vals_lte(datetime, Sys.Date()) |> 
  col_vals_lte(date, Sys.Date()) |>
  interrogate(show_step_label = TRUE)
#> 
#> ── Interrogation Started - there are 2 steps ──────────────────────────────────
#> ✔ Step 1: OK.
#> ✔ Step 2: OK.
#> 
#> ── Interrogation Completed ────────────────────────────────────────────────────

Implementational details

This PR ensures that reading from validation_set$values in the internal function get_values_at_idx() preserves class of the values. This is done by simply replacing unlist(recursive = FALSE) with subsetting [[1]] to this avoids the attribute-stripping behavior of unlist():

unlist(list(Sys.Date()), recursive = FALSE)
#> [1] 19887

Relatedly, the PR also includes a small surgery on interrogate_set() to avoid set operation functions since those strip attributes as well. These have been refactored to use subsetting (x[x %in% y], etc.).

intersect(Sys.Date() + 1:2, Sys.Date() + 2:3)
#> [1] 19889

Related GitHub Issues and PRs

Checklist

Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rich-iannone
Copy link
Member

This is great! Feel free to merge anytime :)

@yjunechoe yjunechoe merged commit c54b035 into rstudio:main Jun 13, 2024
12 of 13 checks passed
@yjunechoe yjunechoe deleted the comparison-preserve-classes branch June 13, 2024 05:39
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.

value is stripped of its class in comparison validation functions Evaluation of datetimes not as expected
2 participants