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

Better alignment of agent$validation_step to rows of get_agent_report(agent) #565

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

yjunechoe
Copy link
Collaborator

This is a small surgery on a single line of get_agent_report():

validation_set <- validation_set[report_tbl$i, ]

The PR just makes the logic of matching rows between validation_set and report_tbl more explicit, via filter() on the matching i columns (where agents always have the invariant of all(diff(agent$validation_set$i) == 1)).

This resolves a cryptic error you get when you remove rows from the agent$validation_set for the purposes of get_agent_report() (ref: #563). Now, we get a better alignment of "rows in $validation_set" and "rows that appear in the {gt} agent report". I'm taking caution to not advertise this as public API (IMO getting the "hide rows" features in {gt} will serve as a better interface to this, when that's implemented). For the time being, the PR simply addresses some confusion about why removing rows in $validation_set does not produce the same effect for get_agent_report() (with an added benefit of making the intent behind the code for this line more explicit).

Using reprex from #563 (hide inactive columns), this now works:

devtools::load_all()

x <- iris |> 
  create_agent() |> 
  col_exists("Petal.Length",
             active = has_columns(iris, Petal.Length)) |> 
  col_exists("Spec",
             active = has_columns(iris, Spec)) |> 
  col_exists("Sepal.Length",
             active = has_columns(iris, Sepal.Length)) |> 
  interrogate()

get_agent_report(x)

image

x$validation_set <- x$validation_set[x$validation_set$eval_active, ]
get_agent_report(x)

image

@yjunechoe yjunechoe linked an issue Sep 8, 2024 that may be closed by this pull request
@rich-iannone rich-iannone self-requested a review September 10, 2024 14:24
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 rich-iannone merged commit a48b998 into rstudio:main Sep 10, 2024
12 checks passed
@yjunechoe yjunechoe deleted the align-validation_step-to-report branch September 10, 2024 14:26
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.

Feature request: Hide inactive tests from validation report
2 participants