Add new (helper) functions for proportions that prepares safe 2x2 cont. tables(s).

[wwojciech]

Fixes #1514

What was done:

1. Added new functions:

• get_complete_cases() (removes observations containing missing values) as a prerequisite for h_prepare_2x2_table(),
• assert_proportion_data() as an assertion used in h_prepare_2x2_table(),
• assert_stratification_compatibility() as an assertion used in s_proportion_diff() and s_test_proportion_diff().

2. Updated the following functions:

• s_proportion_diff(),
• s_test_proportion_diff(),
  so that they use the new functions h_prepare_2x2_table() and assert_stratification_compatibility(),
• check_diff_prop_ci() so that is uses new assert_proportion_data().

3. Resolved issue [Bug]: weights_method argument not used in s_proportion_diff(). #1521.

[wwojciech]

Hi @danielinteractive , can you review this PR please? Thank you!

[danielinteractive]

Thanks @wwojciech , please see first comments re: exporting

[wwojciech]

Thanks @wwojciech , please see first comments re: exporting

Thank you @danielinteractive . Corrected as you suggested.

[danielinteractive]

@wwojciech ok from my side, @Melkiades looks good to you too?

[wwojciech]

Hi @Melkiades , @shajoezhu

Please let me know if you think any further changes/clarifications are required for this PR. Your feedback is greatly appreciated, as this PR is a dependency for some new functions in our package, and development of those functions is currently on hold due to the missing functionality.

@Melkiades - Maybe it is better to place assert_prop_data() in assertions? See ?assertions. What do you think?

Thanks!

[Melkiades]

Thanks for this!!! the helpers are clean and the test coverage is excellent (empty data, single-outcome, unobserved levels, unused strata levels, sparse tables are all covered).

One substantive point plus a few minor issues, left inline. The main thing: issue #1514 frames this as removing duplication across s_test_proportion_diff(), prop_cmh(), prop_fisher(), etc., but as it stands safe_2x2_table() isn't applied at any of those call sites and the same factor(rsp, levels=c("TRUE","FALSE")) + switch(table(...)) logic is still duplicated inline in s_test_proportion_diff(). Could we wire that call site over in this PR?

I also pushed a small fix to NEWS.md: the # tern 0.9.11 heading had landed above the unreleased g_forest() entries (#1499/#1498), which moved them into the already-released 0.9.11 section. I moved the heading below them so they stay under 0.9.11.9000. Feel free to squash/adjust.

[wwojciech]

Thanks for this!!! the helpers are clean and the test coverage is excellent (empty data, single-outcome, unobserved levels, unused strata levels, sparse tables are all covered).

One substantive point plus a few minor issues, left inline. The main thing: issue #1514 frames this as removing duplication across s_test_proportion_diff(), prop_cmh(), prop_fisher(), etc., but as it stands safe_2x2_table() isn't applied at any of those call sites and the same factor(rsp, levels=c("TRUE","FALSE")) + switch(table(...)) logic is still duplicated inline in s_test_proportion_diff(). Could we wire that call site over in this PR?

I also pushed a small fix to NEWS.md: the # tern 0.9.11 heading had landed above the unreleased g_forest() entries (#1499/#1498), which moved them into the already-released 0.9.11 section. I moved the heading below them so they stay under 0.9.11.9000. Feel free to squash/adjust.

Thank you very much @Melkiades, for the detailed review.

I’ve updated the code according to your suggestions. I only kept the stable lifecycle tag, for the reasons I explained above, but I can change it to experimental if this is your strong preference.

Please let me know if you have any further suggestions or would like me to make any additional changes to this PR.

[wwojciech]

Hi @Melkiades , @shajoezhu

Apologies for the reminder, but significant functionality in the package I’m working on depends on this PR, and we have the package release coming up very soon.

I’ve already written a substantial amount of the code on top of this, so I’d really appreciate it if we could merge this as soon as possible to avoid any delays with the release.

Please let me know if there’s anything you’d like me to add or change, or if we’re good to merge it.

Thanks!

[wwojciech]

Hi @munoztd0 , I think it would be reasonable to check on the scda.test.
Can you pl. help with this? Thank you!

[wwojciech]

Thank you, @Melkiades! I renamed the variables according to your suggestion. Please let me know if there’s anything else that needs to be done before this PR can be merged. Thanks!

[munoztd0]

Created a scda.test branch with this new feature branch to check on snapshot tests -> https://github.com/insightsengineering/scda.test/actions/runs/34983541222

(had to tweak a little bit the version dependencies and fork WW branch to make it work nicely in scda.test but everything else is exactly the same in terms of code)

[wwojciech]

Created a scda.test branch with this new feature branch to check on snapshot tests -> https://github.com/insightsengineering/scda.test/actions/runs/34983541222

(had to tweak a little bit the version dependencies and fork WW branch to make it work nicely in scda.test but everything else is exactly the same in terms of code)

Thank you very much indeed @munoztd0 for checking this. This is great!

[munoztd0]

Passed all scda.test snapshot tests

[shajoezhu]

hi @munoztd0 , can you share the link of the PR that you created for this, so we can track it. thanks

[Melkiades]

I am reviewing this. Let me get back at you today @wwojciech @munoztd0

[Melkiades]

@wwojciech This is a clear improvement on what was there. Centralising the 2x2 construction in h_prepare_2x2_table() fixes a real latent bug (the old factor(c(.ref_group[[.var]], df[[.var]]), levels = c("TRUE", "FALSE")) silently produced all NA when .var was a factor), adding cmh_sato to the strata-required set closes another gap, and the 21 tests on the new helper cover more than the code they replace. No existing snapshot moved, which is good evidence the numbers are preserved.

Requesting changes on a few things that change behaviour silently or will fail CI. None of them are design problems.

1. Unstratified method plus strata now errors where it used to be silently ignored. Right call, but it is a behaviour change and needs its own NEWS entry.
2. variables$weights_method is silently dropped, so anyone on the old spelling gets different numbers with no error.
3. Strata columns must now be factors. Character strata used to work.
4. .in_ref_col = NULL silently returns an empty result instead of erroring.
5. _pkgdown.yml lists the assertions topic twice, which will fail the pkgdown build.
6. The duplicated @param strata drops assert_proportion_data()'s own documentation.

Two smaller ones worth folding in while you are here:

• val is not a formal of estimate_proportion_diff() or test_proportion_diff(), so the new feature is only reachable through .... Most users never call s_*() directly.
• expect_error(..., "strat") matches almost any error in this area. expect_snapshot(..., error = TRUE) would lock the real message.

Details inline.

[wwojciech]

@wwojciech This is a clear improvement on what was there. Centralising the 2x2 construction in h_prepare_2x2_table() fixes a real latent bug (the old factor(c(.ref_group[[.var]], df[[.var]]), levels = c("TRUE", "FALSE")) silently produced all NA when .var was a factor), adding cmh_sato to the strata-required set closes another gap, and the 21 tests on the new helper cover more than the code they replace. No existing snapshot moved, which is good evidence the numbers are preserved.

Requesting changes on a few things that change behaviour silently or will fail CI. None of them are design problems.

1. Unstratified method plus strata now errors where it used to be silently ignored. Right call, but it is a behaviour change and needs its own NEWS entry.
2. variables$weights_method is silently dropped, so anyone on the old spelling gets different numbers with no error.
3. Strata columns must now be factors. Character strata used to work.
4. .in_ref_col = NULL silently returns an empty result instead of erroring.
5. _pkgdown.yml lists the assertions topic twice, which will fail the pkgdown build.
6. The duplicated @param strata drops assert_proportion_data()'s own documentation.

Two smaller ones worth folding in while you are here:

• val is not a formal of estimate_proportion_diff() or test_proportion_diff(), so the new feature is only reachable through .... Most users never call s_*() directly.
• expect_error(..., "strat") matches almost any error in this area. expect_snapshot(..., error = TRUE) would lock the real message.

Details inline.

Hi @Melkiades ,

Thank you for your review. I’ve implemented all of your suggestions, except for the one regarding erroring when .in_ref_col and .ref_group are both NULL (see my comments inline, above).

For the minor 2 comments:

• I think introducing val as a formal argument to estimate_proportion_diff() and test_proportion_diff() (and its afuns) would be a relatively bigger change, since the documentation for these functions, as well as for similar functions, currently states that var must be logical. Therefore, I think a reasonable compromise is to keep val only in the lower-level sfuns functions.

• Regarding the error check:
  I agree that this is a compromise between being too specific and not specific enough. However, I think matching "strat" is a reasonable level of specificity for this test.

Please let me know if there is anything else that needs to be addressed before this PR can be merged.

Many thanks!

[Melkiades]

All six points addressed, thanks for turning it around quickly. Re-ran the affected suites on your branch, all green.

Two non-blocking leftovers noted inline: subset.of over set_equal for the variables guard, plus the same guard in s_test_proportion_diff(), and a doc line noting val reaches the layout functions through .... Happy for those to be a follow-up.

Nice work on h_prepare_2x2_table(), it fixes a real latent bug.

[wwojciech]

All six points addressed, thanks for turning it around quickly. Re-ran the affected suites on your branch, all green.

Two non-blocking leftovers noted inline: subset.of over set_equal for the variables guard, plus the same guard in s_test_proportion_diff(), and a doc line noting val reaches the layout functions through .... Happy for those to be a follow-up.

Nice work on h_prepare_2x2_table(), it fixes a real latent bug.

Thank you. I commented on why subset.of over set_equal for the variables guard is not a good choice.
I added guard for variables to s_test_proportion_diff().
Note that val reaches stat function not a layout function, for now, I will leave it without any doc line in layout functions about it.

Thank you for reviewing it.
@shajoezhu - this PR was approved by @Melkiades , so I guess it can be merged? Many thanks again!

[munoztd0]

hi @munoztd0 , can you share the link of the PR that you created for this, so we can track it. thanks

Sorry I didn't do a PR just ran the actions before.

Here is the clean all green PR -> insightsengineering/scda.test#253

[shajoezhu]

All six points addressed, thanks for turning it around quickly. Re-ran the affected suites on your branch, all green.
Two non-blocking leftovers noted inline: subset.of over set_equal for the variables guard, plus the same guard in s_test_proportion_diff(), and a doc line noting val reaches the layout functions through .... Happy for those to be a follow-up.
Nice work on h_prepare_2x2_table(), it fixes a real latent bug.

Thank you. I commented on why subset.of over set_equal for the variables guard is not a good choice. I added guard for variables to s_test_proportion_diff(). Note that val reaches stat function not a layout function, for now, I will leave it without any doc line in layout functions about it.

Thank you for reviewing it. @shajoezhu - this PR was approved by @Melkiades , so I guess it can be merged? Many thanks again!

that's fine. thanks so much. i added you to the repo