fix issues associated with apply_theme() function

[SHAESEN2]

What changes are proposed in this pull request?
Ensure that only the strata present in the theme are displayed.
Other strata are greyed out and legend title is aligned with theme.

Which files are involved in this pull request and what changes were made?

Did you include unit tests for the proposed change/bug fix (https://testthat.r-lib.org/)?
Pending

If there is an GitHub issue associated with this pull request, please provide link.
#388

Checklist for PR reviewer

• PR branch has pulled the most recent updates from main branch. Ensure the pull request branch and your local version match and both have the latest updates from the main branch.
• If a new function was added, function should be included in _pkgdown.yml
• If a bug was fixed, a unit test was added for the bug check
• Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
• Code coverage is suitable for any new functions/features. Review coverage with withr::with_envvar(new = c("NOT_CRAN" = "true"), covr::report()). Before you run, begin a fresh R session without any packages loaded.
• R CMD Check runs without errors, warnings, and notes
• usethis::use_spell_check() runs with no spelling errors in documentation
• Has NEWS.md been updated with the changes from this pull request under the heading indicating the latest version. If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
• Has the version number been incremented using usethis::use_version(which = "dev")
• Has the README been knitted before merging to ensure each PR has the most up-to-date readme?
• Approve Pull Request
• Merge the PR. Please use "Squash and merge".

[ddsjoberg]

@SHAESEN2 i think i can review later tonight. in the meantime, the R CMD Checks are not passing. Do you want to take a look before it's reviewed?

[bailliem]

There is an error being thrown in the Styling_KM_Plots.Rmd vignette running the chunk on line 135

lung_suvival_object %>%
  visR::visr() %>%
  apply_theme()

Error in apply_theme(.) : object 'skipcolor' not found
2. apply_theme(.)

1. lung_suvival_object %>% visR::visr() %>% apply_theme()

[bailliem]

There is an error being thrown in the Styling_KM_Plots.Rmd vignette running the chunk on line 135

lung_suvival_object %>%
  visR::visr() %>%
  apply_theme()

Error in apply_theme(.) : object 'skipcolor' not found 2. apply_theme(.)

1. lung_suvival_object %>% visR::visr() %>% apply_theme()

Ok. If I understand the example its apply an empty theme expecting to revert to the default theme. I'm not sure the example makes a lot of sense. Why would you apply_theme() that is null. Maybe the "fix" here is a warning if no user supplied theme is provided as an arguement.

[SHAESEN2]

There is an error being thrown in the Styling_KM_Plots.Rmd vignette running the chunk on line 135

lung_suvival_object %>%
  visR::visr() %>%
  apply_theme()

Error in apply_theme(.) : object 'skipcolor' not found 2. apply_theme(.)

1. lung_suvival_object %>% visR::visr() %>% apply_theme()

Ok. If I understand the example its apply an empty theme expecting to revert to the default theme. I'm not sure the example makes a lot of sense. Why would you apply_theme() that is null. Maybe the "fix" here is a warning if no user supplied theme is provided as an arguement.

It is to be able to apply sensible defaults as per good visualization practices. However I feel that we should implement them directly rather than making this complex function.

[SHAESEN2]

I think we should add experimental to the function + documentation is still pending

[bailliem]

I think we should add experimental to the function + documentation is still pending

Agree. This seems like a nice to have for now.

[timtreis]

I added a test that covers the behaviour when not enough colours are given and "grey50" is substituted. I'll let Codecov tell me if there's more untested behaviour, but other than that it looks good!

Edit: Now testing for two more use-cases you covered:

a) Use visR palette when theme has no info on strata used in plot (-> had to set skipcolordef to FALSE)
b) Use ggplot palette when strata has more levels than the visR palette elements

[timtreis]

I think this warning can never be triggered because the required situation (more colours than in the visR palette) is caught here and the else if (!skipcolordef) is never entered.

Hey Tim,

I had one situation for which I needed to write that code:

## strata not present but there are too many compared to colors defined
survobj<-
survival::lung %>%
  dplyr::mutate(sex = as.factor(ifelse(sex == 1, "Male", "Female")))  %>%
  dplyr::mutate(status = status - 1) %>%
  dplyr::rename(Age = "age", Sex = "sex", Status = "status", Days = "time")  %>%
  visR::estimate_KM(strata = c("Age", "pat.karno"), CNSR = "Status", AVAL = "Days")

(gg<-survobj%>%
  visR::visr()) %>%
 apply_theme(theme)

[SHAESEN2]

I added a test that covers the behaviour when not enough colours are given and "grey50" is substituted. I'll let Codecov tell me if there's more untested behaviour, but other than that it looks good!

Edit: Now testing for two more use-cases you covered:

a) Use visR palette when theme has no info on strata used in plot (-> had to set skipcolordef to FALSE) b) Use ggplot palette when strata has more levels than the visR palette elements

Thanks for taking the time!!! I'll go through the testing and let you review it once again.

[SHAESEN2]

PR should be finalized. Can someone review?

[bailliem]

Looks good - I'll push a couple of minor changes to the document strings then merge the PR