Fix unconditional exact proportion intervals for tied tables

[Arthur031221]

Fixes #1539

Users of prop_diff_uncond_exact() could get confidence intervals that were too narrow because floating point rounding excluded mathematically tied tables from the tails. Add a 1e-9 tolerance to include those ties.

Builds on @wwojciech's patch and regression table.

Added tests for the reported table and tail membership. Rscript -e 'testthat::test_local(filter = "prop_diff")' passes.

Co-authored-by: Wojtek 11532997+wwojciech@users.noreply.github.com

[wwojciech]

Hi @Arthur031221 !

Thank you for preparing the PR for this.

I think it would be better not to hard-code this tolerance, but instead pass it as an additional argument to worst_case_tail_probability(), e.g. tol = 1e-9. This would make the tolerance explicit and configurable to account for finite-precision arithmetic. It should of course also be exposed in prop_diff_uncond_exact().

Then:

include_table <- if (tail == "upper") {
  t_values >= (t0 - tol)
} else {
  t_values <= (t0 + tol)
}

@danielinteractive - FYI. Please also have a look and let me know if this approach is okay, or if there is a better way of handling this that I may not be aware of. Thanks!

[Arthur031221]

I tested your tol = 1e-9 argument in both functions, and the proportion difference tests pass with new custom and zero tolerance cases in both tails. The PR branch is unchanged pending @danielinteractive's input on the API change.

[wwojciech]

Introduce new tol argument for the respective functions.

[Arthur031221]

Added tol = 1e-9 to prop_diff_uncond_exact() and h_worst_case_tail_probability() and used it for both tail comparisons. Proportion difference tests pass for custom and zero tolerances and invalid inputs.

[danielinteractive]

I found a fix which does not need a tolerance at all. Preparing a separate PR for that now.

[wwojciech]

I found a fix which does not need a tolerance at all. Preparing a separate PR for that now.

Ooo, that is amazing, I am very curios about it :)

[wwojciech]

Hi @Arthur031221 , I noticed that @danielinteractive opened a separate PR to address this issue - #1542
Since Daniel is an author of these functions, lets handle this issue within the PR #1542 and close this one if you do not mind.
Thanks!

[Arthur031221]

Agreed, let's continue in #1542. I'll close this PR. Thanks!

[danielinteractive]

Why do I have the feeling that this was kind of a "robot" generated PR?... 😄

[wwojciech]

Why do I have the feeling that this was kind of a "robot" generated PR?... 😄

I had a similar feeling, although I wasn’t sure about it. It would be quite disappointing if that were actually the case.

@shajoezhu - could you please shed some light on what’s happening here? I’d be genuinely disappointed if this was indeed generated by a bot. I’ve spent quite a bit of time and effort reviewing and discussing this, and I’d hate to find out that I was essentially talking to a bot rather than a person.