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

Refactor printing of steps #871

Merged
merged 100 commits into from
Jan 11, 2022
Merged

Refactor printing of steps #871

merged 100 commits into from
Jan 11, 2022

Conversation

EmilHvitfeldt
Copy link
Member

@EmilHvitfeldt EmilHvitfeldt commented Dec 15, 2021

Problem

We want to move over to using {cli} for printing in the package #426. However the current way printing is done in this package + extension packages doesn't allow for an easy transition. {cli} and cat() doesn't work well together. The reprex below neatly outlines the problem with trying to move to {cli} using extension packages.

Suppose that we completely swap to {cli} but the user uses an older version of an extension package. Then when an extension step is being used it will contain a cat() call, messing with the print method. (SMOTE based on is being printed before everything else`)

print.step_center <- function() {
  cli::cli_text("Centering for ")
  printer(c("mpg", "cyl"))
}
print.step_scale <- function() {
  cli::cli_text("Scaling for")
  printer(c("mpg", "cyl"))
}
print.step_smote <- function() {
  cat("SMOTE based on ")
  printer(c("cyl"))
}

printer <- function(cols) {
  cli::cli_text(glue::glue_collapse(cols, sep = ", "))
}

print.recipe <- function() {
  children <- list(
    print.step_center,
    print.step_scale,
    print.step_smote
  )
  
  cli::cli({
    for (child in children) {
      child()
    }
  })
}

print.recipe()
#> SMOTE based on
#> Centering for
#> mpg, cyl
#> Scaling for
#> mpg, cyl
#> cyl

Created on 2021-12-15 by the reprex package (v2.0.1)

Solution / Plan

My proposed solution is to move to {cli} over two releases.

version 0.1.18

  • Include print_step() function, superseding printer(). This function will keep printing output unchanged, but moves the responsibility of step title printing.
  • {recipes} and extension packages move to adopt print_step() in favor of printer() whenever possible. With the goal of eliminating all calls to cat() from print.step_*() functions across all packages.
# Old
print.step_pls <- function(x, width = max(20, options()$width - 35), ...) {
  cat("PLS feature extraction with ")
  printer(x$columns, x$terms, x$trained, width = width)
  invisible(x)
}

# New
print.step_pls <- function(x, width = max(20, options()$width - 35), ...) {
  title <- "PLS feature extraction with "
  print_step(x$columns, x$terms, x$trained, width = width, title = title)
  invisible(x)
}

version 0.1.19

print.recipe() and print_step() move from cat() to {cli}.

This is not a perfect solution, but it reduces the change someone gets butchered printing from recipes as they would need to have version 0.1.19 recipes, but a pre 0.1.18 extension package.

@EmilHvitfeldt
Copy link
Member Author

This should be all the changes here in {recipes} to the steps. I tried to minimally change the the output. Many more steps now correctly print "" when no variables are selected

@EmilHvitfeldt
Copy link
Member Author

Finished running revdepcheck

Results:

# Platform

|field    |value                          |
|:--------|:------------------------------|
|version  |R version 4.1.1 (2021-08-10)   |
|os       |macOS Big Sur 11.3.1           |
|system   |x86_64, darwin17.0             |
|ui       |RStudio                        |
|language |(EN)                           |
|collate  |en_US.UTF-8                    |
|ctype    |en_US.UTF-8                    |
|tz       |America/Los_Angeles            |
|date     |2021-12-21                     |
|rstudio  |1.5.216 Ghost Orchid (desktop) |
|pandoc   |2.5 @ /usr/local/bin/pandoc    |

# Dependencies

|package    |old    |new         |Δ  |
|:----------|:------|:-----------|:--|
|recipes    |0.1.17 |0.1.17.9000 |*  |
|parallelly |NA     |1.30.0      |*  |
|progressr  |NA     |0.10.0      |*  |

# Revdeps

## New problems (1)

|package                            |version |error |warning |note |
|:----------------------------------|:-------|:-----|:-------|:----|
|[usemodels](problems.md#usemodels) |0.1.0   |      |__+1__  |2    |

The new warning is: which doesn't have anything to do with this PR

*   checking whether package ‘usemodels’ can be installed ... WARNING
    ```
    Found the following significant warnings:
      Warning: replacing previous import ‘recipes::tune_args’ by ‘tune::tune_args’ when loading ‘usemodels’
    See ‘/Users/emilhvitfeldthansen/Desktop/recipes/revdep/checks.noindex/usemodels/new/usemodels.Rcheck/00install.out’ for details.
    ```

@EmilHvitfeldt EmilHvitfeldt marked this pull request as ready for review December 21, 2021 23:02
@topepo topepo merged commit 55ecda0 into main Jan 11, 2022
@topepo topepo deleted the printing-refactor branch January 11, 2022 22:25
topepo added a commit that referenced this pull request Jan 12, 2022
topepo added a commit that referenced this pull request Jan 13, 2022
* changes for #823

* use kernlab directly instead if dimRed

* use fastICA directly instead of dimRed

* copious skips

* add seed argument for ICA and some code refactoring

* Update R/misc.R

Co-authored-by: Davis Vaughan <davis@rstudio.com>

* uncomment example code

* rename calling function

* small new change

* roll back kpca change (bad update form main)

* updated print method for #871

* changes based on reviewer feedback

Co-authored-by: Davis Vaughan <davis@rstudio.com>
@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants