Option to specify open and closed intervals for CI table #32
Conversation
- CI_bracket_open, CI_bracket_close - these arguments allow users to set the format of the open and closing interval (brackets or parenthesis) Changed warning to a message for extra columns - Think this could be somewhat of a common occurence, and dont think it deserves a warning since it doesn't impact anything
|
@seth127 FYI I tested these args with multiple simulations as well, but didn't see a need to add a test for that (runs through the same section of code regardless)
|
|
Pasting the locally run tests since drone doesn't run the plotting tests: devtools::test()> devtools::test()
ℹ Testing pmforest
✔ | F W S OK | Context
✔ | 27 | base-plot [8.8s]
✔ | 3 | input-data [0.1s]
✔ | 2 | nsim-plot [1.7s]
✔ | 5 | summary [0.9s]
══ Results ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 11.6 s
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 37 ]R CMD Check==> devtools::check()
══ Documenting ════════════════════════════════════════════════════════════
ℹ Updating pmforest documentation
ℹ Loading pmforest
══ Building ═══════════════════════════════════════════════════════════════
Setting env vars:
• CFLAGS : -Wall -pedantic -fdiagnostics-color=always
• CXXFLAGS : -Wall -pedantic -fdiagnostics-color=always
• CXX11FLAGS: -Wall -pedantic -fdiagnostics-color=always
• CXX14FLAGS: -Wall -pedantic -fdiagnostics-color=always
• CXX17FLAGS: -Wall -pedantic -fdiagnostics-color=always
• CXX20FLAGS: -Wall -pedantic -fdiagnostics-color=always
── R CMD build ────────────────────────────────────────────────────────────
✔ checking for file ‘/data/Projects/package_dev/pmforest/DESCRIPTION’ ...
─ preparing ‘pmforest’: (10.6s)
✔ checking DESCRIPTION meta-information ...
─ installing the package to build vignettes
✔ creating vignettes (17.9s)
─ checking for LF line-endings in source and make files and shell scripts (768ms)
─ checking for empty or unneeded directories
─ building ‘pmforest_0.1.1.tar.gz’
══ Checking ═══════════════════════════════════════════════════════════════
Setting env vars:
• _R_CHECK_CRAN_INCOMING_USE_ASPELL_ : TRUE
• _R_CHECK_CRAN_INCOMING_REMOTE_ : FALSE
• _R_CHECK_CRAN_INCOMING_ : FALSE
• _R_CHECK_FORCE_SUGGESTS_ : FALSE
• _R_CHECK_PACKAGES_USED_IGNORE_UNUSED_IMPORTS_: FALSE
• NOT_CRAN : true
── R CMD check ────────────────────────────────────────────────────────────
─ using log directory ‘/data/Projects/package_dev/pmforest.Rcheck’
─ using R version 4.1.3 (2022-03-10)
─ using platform: x86_64-pc-linux-gnu (64-bit)
─ using session charset: UTF-8
─ using options ‘--no-manual --as-cran’
✔ checking for file ‘pmforest/DESCRIPTION’
─ this is package ‘pmforest’ version ‘0.1.1’
─ package encoding: UTF-8
✔ checking package namespace information ...
✔ checking package dependencies (3.4s)
✔ checking if this is a source package
✔ checking if there is a namespace
✔ checking for executable files ...
✔ checking for hidden files and directories
✔ checking for portable file names ...
✔ checking for sufficient/correct file permissions ...
✔ checking whether package ‘pmforest’ can be installed (4.6s)
✔ checking installed package size ...
✔ checking package directory ...
✔ checking for future file timestamps ...
✔ checking ‘build’ directory
✔ checking DESCRIPTION meta-information ...
✔ checking top-level files
✔ checking for left-over files
✔ checking index information ...
✔ checking package subdirectories ...
✔ checking R files for non-ASCII characters ...
✔ checking R files for syntax errors ...
✔ checking whether the package can be loaded (1.1s)
✔ checking whether the package can be loaded with stated dependencies (1s)
✔ checking whether the package can be unloaded cleanly (1s)
✔ checking whether the namespace can be loaded with stated dependencies (947ms)
✔ checking whether the namespace can be unloaded cleanly (1.1s)
✔ checking loading without being on the library search path (1.1s)
✔ checking dependencies in R code (1.1s)
✔ checking S3 generic/method consistency (2.4s)
✔ checking replacement functions (952ms)
✔ checking foreign function calls (1.1s)
✔ checking R code for possible problems (6.6s)
✔ checking Rd files ...
✔ checking Rd metadata ...
✔ checking Rd line widths ...
✔ checking Rd cross-references ...
✔ checking for missing documentation entries (945ms)
✔ checking for code/documentation mismatches (3s)
✔ checking Rd \usage sections (2.6s)
✔ checking Rd contents ...
✔ checking for unstated dependencies in examples ...
✔ checking installed files from ‘inst/doc’ ...
✔ checking files in ‘vignettes’ ...
─ checking examples ... NONE
✔ checking for unstated dependencies in ‘tests’ ...
─ checking tests ...
✔ Running ‘testthat.R’ [13s/13s] (12.9s)
✔ checking for unstated dependencies in vignettes ...
✔ checking package vignettes in ‘inst/doc’ ...
✔ checking re-building of vignette outputs (13.5s)
✔ checking for non-standard things in the check directory
✔ checking for detritus in the temp directory
── R CMD check results ──────────────────────────────── pmforest 0.1.1 ────
Duration: 1m 3.7s
0 errors ✔ | 0 warnings ✔ | 0 notes ✔
R CMD check succeeded |
There was a problem hiding this comment.
The primary code change looks fine to me, and I agree with the rationale you gave in 98538ad for the unrelated warning -> message demotion.
But here's an alternative approach for you to consider: rather than introducing two new arguments, add one (say CI_format) that is a format specifier for the string that will be constructed as sprintf(CI_format, lb, ub). To retain the current behavior, the default value would be "[%s, %s]".
I like this better because:
- it only adds one argument rather than two, which is nice given how conceptually coupled these values are
- it takes care of potential future requests (e.g., "I'd like to use just a space instead of a comma")
- it uses a familiar syntax
The main downside I see is that it'd probably be surprising to the user that lb and ub are strings constructed with pmtables::sig, so they'd need to use the digits argument rather than, say, %.2g to control that. Similarly, they wouldn't be able to use alternative numeric formats, such as %f or %e.
That's perhaps okay. But given that this package is young and at 0.1.1, I think it'd be better to go farther:
-
introduce the new
CI_formatargument with a default value of "[%.3g, %.3g]" -
deprecate
digits, setting its default toNULL. To accommodate any existing code, add a compatibility kludge that checks whetherdigitsisNULL. If it isn't, give a deprecation warning and change the format specifier so thatdigitsis honored. For example, ifdigits = 2, the format specified would change the format specifier to "[%.2g, %.2g]". (If the format specifier isn't the default "[%.3g, %.3g]", it is safe to error because no code in the wild would be passingCI_formatyet.)
What do you think?
| @@ -308,6 +308,31 @@ describe("Base plots", { | |||
| vdiffr::expect_doppelganger("Character interpretation of numeric group_level", plt2) | |||
|
|
|||
| }) | |||
|
|
|||
| it("Change CI interval format [PMF-PLOT-025]", { | |||
There was a problem hiding this comment.
I think you're holding off on reqs because things are up in the air, but is it worth adding the test ID if that's the case?
There was a problem hiding this comment.
Yes that's correct - but yeah it wouldn't hurt to add
There was a problem hiding this comment.
Hmm, just to be clear, I was referring to the test ID that you are adding here, and suggesting that it was not worth doing.
tests/testthat/test-base-plot.R
Outdated
| error_msg <- capture_error( | ||
| plot_forest(data = sumData, CI_bracket_open = "]", CI_bracket_close = "]") | ||
| ) | ||
| expect_true( |
There was a problem hiding this comment.
Could this be simplified by using expect_error?
There was a problem hiding this comment.
I couldnt get that to work because the error message used the weird curly quotes. I could grab a section of the error message, but the curly quotes captured the main part I was looking for. Happy to adjust if you see a better approach
There was a problem hiding this comment.
Edit: replied to the wrong thread, but leaving it so I dont ping everyone again.
- it only adds one argument rather than two, which is nice given how conceptually coupled these values are
- it takes care of potential future requests (e.g., "I'd like to use just a space instead of a comma")
- it uses a familiar syntax
I actually went through a bunch of options when deciding what would make the most sense. I asked @andersone1, @michaelmcd18, and @graceannobrien and settled on an approach that they liked. I like your suggestion, but as someone who doesn't use the package all too much, i'd prefer to get their opinion on it (Or a scientist - was told Katherine might be a good resource). If it's desirable to them or they dont have a strong opinion, i'd vote to do something like you're suggesting (mainly to give user's more options with fewer arguments). I am a little worried some scientists wouldn't like to specify it this way though.
In terms of deprecating digits, i'd like to know more about what you're envisioning down the road. As you note, the package is early in its development. Given that does it make sense to introduce kludge to support older behavior; and if so, how long (or number of versions) do we do this for? Seems like we'd remove it at some point, but just curious what the standard is for that.
There was a problem hiding this comment.
I couldnt get that to work because the error message used the weird curly quotes
Hmm, I'm not sure what you're referring to. Fwiw this worked fine on my end:
diff --git a/tests/testthat/test-base-plot.R b/tests/testthat/test-base-plot.R
index 98cdf0d..c627037 100644
--- a/tests/testthat/test-base-plot.R
+++ b/tests/testthat/test-base-plot.R
@@ -325,13 +325,10 @@ describe("Base plots", {
vdiffr::expect_doppelganger("Change CI interval format - change both", plt)
# error
- error_msg <- capture_error(
- plot_forest(data = sumData, CI_bracket_open = "]", CI_bracket_close = "]")
- )
- expect_true(
- grepl("'arg' should be one of", error_msg$message)
+ expect_error(
+ plot_forest(data = sumData, CI_bracket_open = "]", CI_bracket_close = "]"),
+ regexp = "'arg' should be one of"
)
-
})
})
With a pre 1.0.0 version under semantic versioning, you're free to make whatever breaking changes you want. However, if we know that has been used in real project work already, I would recommend adding the simple kludge to give users a window to update rather than crashing with an "unused argument" error when they pass
In my view it doesn't really matter too much. If it's a burden in any way, you'll be reminded and remove it. And if not, well then it's not really causing any issue. But I'd say warning for one release (or more specifically one release that's made it to MPN) and then removing it in the next is fine. |
Thanks for the extra info. Too bad that discussion wasn't capture in a public spot :)
Sounds like a good idea.
Hmm, because you think C-style format strings would be unfamiliar? Or some other reason? |
Yeah - good point lol
Yes, that's my main concern - not that they couldn't figure it out, but that they might find it annoying/frustrating or something |
|
Revisiting this PR after some time. Requesting @andersone1 and @graceannobrien for review (whoever gets to it first) because you guys were present for the conversation we had about the implementation of this.
|
This PR introduces two new arguments,
CI_bracket_open&CI_bracket_close, which allow users to specify whether to use brackets ([]) or parentheses (()) for the opening and closing interval respectively. Users can mix and match between the two options to denote whether the interval includes the endpoint on the left or right of the interval, though it primarily serves as a formatting option.Examples
using parentheses
mixing and matching
closes #31