-
Notifications
You must be signed in to change notification settings - Fork 46
Add print methods for control objects with formatted output using cli #1108
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
This is all nice already! I would suggest, if you have time to spare, to take a look at the cli classes we can use. This should allow you to reduce some of the code you have in |
|
Additional if we wanted to only show what is non-default we could add a Add the following line inside the loop if (identical(value, default[[field]])) {
next
}and lastly pass in the corresponding objects in the uses. So print.control_grid <- function(x, ...) {
cli::cli_text("{.emph Grid/resamples control object}")
--- print_control_settings(x)
+++ print_control_settings(x, default = control_grid())
invisible(x)
}I'm looping @topepo in on this decision as i don't have very strong feelings here. |
|
Thanks - implemented. The function print is still kind of ugly, but the rest now use the cli classes better. |
|
I thought that we could do something fancy with |
tests/testthat/test-checks.R
Outdated
| expect_s3_class(control_bayes(), "control_bayes") | ||
| }) | ||
|
|
||
| test_that("control object print methods", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this should live in test-control.R
| cli::cli_bullets(c(" " = "{.arg {field}}: <function>")) | ||
| } else if (inherits(value, "tune_backend_options")) { | ||
| cli::cli_bullets(c(" " = "{.arg {field}}: <backend_options>")) | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a branch for NULL values before the last that is:
else if (is.null(value)) {
cli::cli_bullets(c(" " = "{.arg {field}}: NULL"))
}We can then get the output without the empty spaces:
Grid/resamples control object
`verbose`: FALSE
`allow_par`: TRUE
`extract`: NULL
`save_pred`: FALSE
`pkgs`: NULL
`save_workflow`: FALSE
`event_level`: "first"
`parallel_over`: NULL
`backend_options`: NULL
`workflow_size`: 100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in latest commit
- Add NULL value handling to print_control_settings function This prevents empty output for NULL values, showing them explicitly as "NULL" - Move control object print method tests from test-checks.R to test-control.R for better organization - Remove corresponding snapshots from checks.md (will be regenerated in control.md) These changes address topepo's review feedback on PR tidymodels#1108.
|
Cool, just implemented NULL handling in the latest commit and put tests in the right place. Let me know if there's anything else. Ignore the claude reference above - got some free credits dumped into my account that I figured I'd see what it could do and didn't realize it would go off and put its changes on git. |
Closes #1051 by adding a cli-based printout of control objects. Went pretty simple, just a cli bulleted list of all parameters. Doing all is up for debate - I figure it's a bit nicer in case users don't know defaults off the cuff, but can see the argument for only non-default values, the prints do get a little verbose.