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

Use consistent conf.level argument #133

Closed
ddsjoberg opened this issue Apr 21, 2024 · 2 comments · Fixed by #142
Closed

Use consistent conf.level argument #133

ddsjoberg opened this issue Apr 21, 2024 · 2 comments · Fixed by #142
Assignees
@zdz2101
Labels
ARD sme

Comments

@ddsjoberg
Copy link
Collaborator

many ard_*() have a conf.level argument. But some have a similar argument that is handled via the ....

For example, ard_effectsize_cohens_d(...), where the ... are passed to effectsize::cohens_d(...), where there is a ci argument. In this case, we should surface this argument to ard_effectsize_cohens_d(conf.level).

Need to inspect the other functions to see where else this occurs.
@zdz2101
Copy link
Contributor

zdz2101 commented May 1, 2024 •
edited
Loading

@ddsjoberg for a function like ard_stats_fisher_test() , do we want conf.level more forward/front facing as an argument instead of inherently part of the passed arguments of ... ? Doesn't need renaming but it makes it feel like some functions we have conf.level as an argument and some not, for example it's brought to the surface for the proportion_ci.R and ard_survey_svyttest.R functions,

would affect(s):

  • ard_stats_fisher_test
  • ard_stats_prop_test
  • ard_stats_t_test
  • ard_stats_wilcox_test

@ddsjoberg
Copy link
Collaborator Author

Yes, I think we can surface the argument in these cases. Good thought!

@zdz2101 zdz2101 mentioned this issue May 2, 2024
Merged
13 tasks
@zdz2101 zdz2101 linked a pull request May 2, 2024 that will close this issue
Closes #133 proliferate conf.level argument as appropriate across package #142
Merged
13 tasks
ddsjoberg added a commit that referenced this issue May 3, 2024
@zdz2101 @ddsjoberg
Closes #133 proliferate conf.level argument as appropriate across p…
817694d 
…ackage (#142)

**What changes are proposed in this pull request?**
Adds `conf.level` argument to variety of functions for consistency

Provide more detail here as needed.

Closes #133


--------------------------------------------------------------------------------

Pre-review Checklist (if item does not apply, mark is as complete)
- [x] **All** GitHub Action workflows pass with a ✅
- [x] PR branch has pulled the most recent updates from master branch:
`usethis::pr_merge_main()`
- [x] If a bug was fixed, a unit test was added.
- [x] If a new `ard_*()` function was added, it passes the ARD
structural checks from `cards::check_ard_structure()`.
- [x] If a new `ard_*()` function was added, `set_cli_abort_call()` has
been set.
- [x] If a new `ard_*()` function was added and it depends on another
package (such as, `broom`), `is_pkg_installed("broom", reference_pkg =
"cardx")` has been set in the function call and the following added to
the roxygen comments: `@examplesIf
do.call(asNamespace("cardx")$is_pkg_installed, list(pkg = "broom"",
reference_pkg = "cardx"))`
- [x] Code coverage is suitable for any new functions/features
(generally, 100% coverage for new code): `devtools::test_coverage()`

Reviewer Checklist (if item does not apply, mark is as complete)

- [x] If a bug was fixed, a unit test was added.
- [x] Code coverage is suitable for any new functions/features:
`devtools::test_coverage()`

When the branch is ready to be merged:
- [x] Update `NEWS.md` with the changes from this pull request under the
heading "`# cardx (development version)`". If there is an issue
associated with the pull request, reference it in parentheses at the end
update (see `NEWS.md` for examples).
- [ ] **All** GitHub Action workflows pass with a ✅
- [ ] Approve Pull Request
- [ ] Merge the PR. Please use "Squash and merge" or "Rebase and merge".

---------

Co-authored-by: Daniel Sjoberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zdz2101 zdz2101

Labels
ARD sme
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Closes #133 proliferate conf.level argument as appropriate across package insightsengineering/cardx
2 participants
@ddsjoberg @zdz2101