Skip to content

Fix docs (especially for coxph) and UI messaging - #261

Merged
juliasilge merged 10 commits into
mainfrom
fix-coxph-docs
Apr 26, 2023
Merged

juliasilge merged 10 commits into
mainfrom
fix-coxph-docs

Conversation

@juliasilge

Copy link
Copy Markdown
Member

Closes #260

This PR mainly

  • adds a disclaimer to the coxph docs about not using butcher from anywhere but the global env
  • changes the UI messaging to tell folks when their butchered object is actually larger

Turns out we didn't have markdown = TRUE set yet so this PR makes that change and updates the needed docs as well.

Comment thread R/ui.R
rel <- format(rel, big.mark = ",", scientific = FALSE)
if (length(rel) == 1) {
if (rel <= 0) {
if (isTRUE(all.equal(old, new))) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Floating point math scares me. 😱 Any better options for this?

Comment thread tests/testthat/test-ui.R
assess_object(small, small)
assess_object(small, big)
},
transform = function(x) gsub(obj_size_diff, "<redacted>", x, fixed = TRUE)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure if it's possible that the memory for these objects would be different in the future or on a different platform, but let's just be safe.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup, I think that's a good idea.👍

@simonpcouch simonpcouch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The tests are wonderfully concise and I'm appreciating the way you've phrased these docs. Thumbs up from me. :)

Comment thread tests/testthat/test-ui.R
assess_object(small, small)
assess_object(small, big)
},
transform = function(x) gsub(obj_size_diff, "<redacted>", x, fixed = TRUE)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup, I think that's a good idea.👍

Comment thread R/ui.R Outdated
cli::cli_alert_danger("Could not add {.cls {class_name}} class")
abs_mem <- format(abs(mem), big.mark = ",", scientific = FALSE)
if (mem < 0) {
cli::cli_alert_danger("The butchered object is {.val {abs_mem}} larger than the original. Do not butcher.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With this PR, I see:

> res_via_func <- coxph_formula_butchered(example_data)
✖ The butchered object is "920 B" larger than the original. Do not butcher.

(with the blue formatting for 920 B). The quotes caught my eye--perhaps we could use .field or a straight up glue substitution here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, I see that Memory released: "533.24 kB" uses this pattern too. Maybe this is fine!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something about the phrasing this message feels strange to me but maybe that's the purpose—raise an eyebrow in this weird (maybe coxph-specific?) situation.

@juliasilge juliasilge Apr 26, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree with you that the BLUE plus QUOTES looks weird. What if I change them both to just .field? That makes them green with no quote marks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm game for that!

@hfrick hfrick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good wording and example! 🙏

@juliasilge
juliasilge marked this pull request as ready for review April 26, 2023 20:09
@juliasilge
juliasilge merged commit 7e162c9 into main Apr 26, 2023
@juliasilge
juliasilge deleted the fix-coxph-docs branch April 26, 2023 21:06
@github-actions

Copy link
Copy Markdown
Contributor

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 May 11, 2023
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.

Problems with survival::coxph() and env handling

3 participants