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.

Sign up for GitHub

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

Jump to bottom

Zooming in on figure with coord_cartesean() #437

Merged
merged 7 commits into from
Jul 12, 2022
Merged

Zooming in on figure with coord_cartesean() #437

merged 7 commits into from
Jul 12, 2022

Conversation

ddsjoberg
Copy link
Collaborator

@ddsjoberg ddsjoberg commented Jun 29, 2022 •
edited
Loading

What changes are proposed in this pull request?

  • Zooming in on figure with coord_cartesean() instead of using xlim= and ylim=. The latter first remove data outside the limits, then construct the line. Zooming in constructs the full line, then zooms into the limits. This is useful because the risktable often reports estimates near the end of a KM figure, but the line is cutoff and not shown at the last timepoint. (KM figures cut off at ends with visr(x_ticks=) #402)

Did you include unit tests for the proposed change/bug fix (https://testthat.r-lib.org/)?
The checks ran without issue on my machine. I am not sure what unit tests can be added to check the results at the moment. I think this will be checked when we implement snapshots in testthat.

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

Here is what the update looks like:

library(visR)

estimate_KM(
  data = survival::lung %>% dplyr::mutate(time = ifelse(time > 1000, 2001, time)),
  formula = Surv(time, status) ~ 1
) %>%
  visr(x_ticks = seq(0, 2000, by = 200)) %>%
  add_risktable()

Created on 2022-06-28 by the reprex package (v2.0.1)

And this is what was previously shown:

I also added the optional development mode from devtools. If any devs use dev mode, it'll be activated. If you don't, there is no change at all.

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")
  • Run devtools::build_readme() to build the README.md file.
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge".

@ddsjoberg ddsjoberg marked this pull request as ready for review June 29, 2022 01:58
@SHAESEN2
Copy link
Collaborator

SHAESEN2 commented Jul 4, 2022

This makes sense !

SHAESEN2
SHAESEN2 requested changes Jul 4, 2022
View reviewed changes
Copy link
Collaborator

@SHAESEN2 SHAESEN2 left a comment

Choose a reason for hiding this comment

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

Please include testing for this. You can choose to use a visual compare or deconstruct the ggplot to question what is being displayed.

@ddsjoberg
Copy link
Collaborator Author

I did some reading on snapshots and vdiffr. It seems that vdiffr is a special type of testthat snapshot. I added a vdiffr snapshot test for this pull request. @SHAESEN2

I saw that the _snaps folder, however, was included in the gitignore. I don't think the snapshots can be compared on GH Actions when this folder is not included in the repo. I navigated to the ggplot2 repo, and their snaps folder is indeed uploaded to GH. So I removed the folder from the gitignore, and now all the vidffr snapshots are also checked on GH Actions.

@ddsjoberg ddsjoberg mentioned this pull request Jul 4, 2022
Closed
@SHAESEN2
Copy link
Collaborator

SHAESEN2 commented Jul 7, 2022

I think we should then also update all our testing with snapshots because most of it is out of date.

SHAESEN2
SHAESEN2 reviewed Jul 7, 2022
View reviewed changes
tests/testthat/test-visr_survfit.R
@@ -47,6 +47,8 @@
#' T5. The final object is a ggplot of class `ggsurvfit`.
#' T5.1 The final object is a ggplot of class `ggplot`.
#' T5.2 The final object is a ggplot of class `ggsurvfit`.
#' T6. The final object does not exclude parts of KM estimate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a bit vague. Could you rephrase? SOmething with using xlim limits the display of the estimate to ... without ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

again, if you want this text updated, feel free to do it yourself OR provide the text exactly as you want to see it. I do not find the current text vague.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because it is vague, I dont know what you are testing:
The final object does not exclude parts of KM estimate. Which parts? and what is the purpose? Did you mean that the object will display all available time points even if the x axis argument is used?

Copy link
Collaborator Author

@ddsjoberg ddsjoberg Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This entire PR makes one small change, and from my POV it seems like a bad-faith argument that it's unclear what is intended to be tested here (even if there were no text describing it). If you would like something else here, go ahead and update it sometime.

SHAESEN2
SHAESEN2 reviewed Jul 7, 2022
View reviewed changes
tests/testthat/test-visr_survfit.R

testthat::context("visr_plot - T6. The final object does not exclude parts of KM estimate.")

testthat::test_that("T6.1 The final object zooms and does not exclude trialing pieces of lines.", {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a big fan of creating the actual and test plot with similar code. We should update this throughout the package. Could we somehow get the line information and ensure it is displayed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what you mean. The snap (aka the svg image of the ggplot) is created from the code and the unit test will always correspond to the code....

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be independent. Perhaps we can organize a spring where we implement this. For now OK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK sounds like a good plan.

@ddsjoberg
Copy link
Collaborator Author

I think we should then also update all our testing with snapshots because most of it is out of date.

All the snaps were updated...what exactly are you requesting here? @SHAESEN2

@ddsjoberg
Copy link
Collaborator Author

ddsjoberg commented Jul 11, 2022 •
edited
Loading

@SHAESEN2 @bailliem are there updates to the unit testing needed before this is merged?

@ddsjoberg ddsjoberg mentioned this pull request Jul 12, 2022
Merged
12 tasks
@ddsjoberg ddsjoberg merged commit 50ec226 into main Jul 12, 2022
@ddsjoberg ddsjoberg deleted the zoom_zoom_zoom branch July 12, 2022 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SHAESEN2 SHAESEN2 SHAESEN2 requested changes

@bailliem bailliem Awaiting requested review from bailliem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KM figures cut off at ends with visr(x_ticks=)
2 participants
@ddsjoberg @SHAESEN2