Solves #1535: Refactored prop_diff_cmh() and its helper functions

[wwojciech]

Fixed #1535

Summary of the main changes

• h_diff_cmh():

  • Renamed to h_prop_cmh().
  • Refactored and removed diff_est from the output list so that this function is concerned only with proportions and their inference.

• h_diff_cmh_se():

  • Renamed to h_cmh_sato_var().
  • Refactored.
  • Moved the "standard" SE computations to prop_diff_cmh().

• h_miettinen_nurminen_var_est():

  • Renamed to h_miettinen_nurminen_var().
  • Refactored.

• h_miettinen_nurminen_stratified_ci():

  • Added as a new function extracted from prop_diff_cmh().

• prop_cmh():

  • Modified to use h_cmh_sato_var().

• Added uniroot_catch_na().

[wwojciech]

Hi @munoztd0 - would it be possible to run the scda tests against this PR?

[wwojciech]

For reviewers (@danielinteractive , @Melkiades ).

Please specifically check the test "test_proportion_diff edge case: all responder by CMH with Sato variance estimator" in test-test_proportion_diff.R that now has NA in the rcell instead of 1 for p-value.

I’m also open to changing this to 1, since the value here is more of a convention for a degenerate case rather than something directly produced by the CMH/Sato test itself.

Let me know what do you think.

[danielinteractive]

Thanks @wwojciech , please see comments below.

General recommendation: not everywhere where we divide something by a variable we have to first check whether that thing is positive etc. We can leave such things in many cases to the R functions, which usually do a good job with infinite, NaN etc. Sometimes we need to do something manually, but not all the time. Otherwise it becomes too hard to read and maintain.

[wwojciech]

Thanks @wwojciech , please see comments below.

General recommendation: not everywhere where we divide something by a variable we have to first check whether that thing is positive etc. We can leave such things in many cases to the R functions, which usually do a good job with infinite, NaN etc. Sometimes we need to do something manually, but not all the time. Otherwise it becomes too hard to read and maintain.

Thank you @danielinteractive for the review and for raising this point. I agree that we don't need to explicitly check for undefined expressions everywhere, and we should absolutely avoid cluttering the code where R's defaults work well.
However, for this specific case, I think there is a strong domain-specific reason to handle it explicitly:

1. General case
   When dealing with values that are not exact, or where the exactness is unknown, I agree that it is better to rely on R's default behavior and its IEEE 754 semantics. Similarly, letting R handle 0/0 by returning NaN is perfectly fine across any data type.

2. Count Data (Our Case) and Inf
   Here, we are dealing with exact counts. A 0 has a concrete domain meaning (e.g., zero observations or zero events), rather than representing a floating-point value that happens to be very close to zero. Because of this, the calculus concept of a limit lim_{x -> 0} a/x = \infty (for some finite, positive a) doesn't apply cleanly to our metrics.

   Allowing Inf to propagate can easily lead to downstream data corruption, skewed metrics, or silent errors in the standard R functions. For example:

   sum(c(1, 2, 5 / 0))  # Returns Inf
   mean(c(1, 2, 5 / 0)) # Returns Inf

   In our context, if a denominator count is 0, the resulting metric should ideally be treated as NA so that standard statistical summaries don't get skewed.

   (PS. This is consistent with C, Java, and Python, where integer division by zero is either undefined or raises a runtime error. R only defaults to Inf because its / operator implicitly treats all numbers as floating-point, which masks what is fundamentally a data-completeness issue for our discrete metrics).

I will update the code to handle this cleanly and address the other comments inline. Let me know if that sounds like a fair compromise.

[danielinteractive]

@wwojciech yeah for those cases with integer counts I agree it might make sometimes more sense to use NA manually. But for most cases in your PR it was about floating point numbers. Therefore I felt that it is useful to summarize in my general comment.

[wwojciech]

@wwojciech yeah for those cases with integer counts I agree it might make sometimes more sense to use NA manually. But for most cases in your PR it was about floating point numbers. Therefore I felt that it is useful to summarize in my general comment.

Sure, thanks @danielinteractive. Let me update the code and remove the explicit handling where it’s unnecessary.

[Melkiades]

Dear @wwojciech, thanks for the addition. As far as I could understand from the issue, this was meant to be a bug fix, while I see major refactoring. Please consider dividing this in 2 PRs, one with the bug fix, and the other with the refactoring. For example, the change of order and function names should be in a separate PR as it makes difficult to read the PR. Also remember that if it is exported you need the deprecation cycle

[wwojciech]

Dear @wwojciech, thanks for the addition. As far as I could understand from the issue, this was meant to be a bug fix, while I see major refactoring. Please consider dividing this in 2 PRs, one with the bug fix, and the other with the refactoring. For example, the change of order and function names should be in a separate PR as it makes difficult to read the PR. Also remember that if it is exported you need the deprecation cycle

Thank you @Melkiades for quick comment.

Yeah, if we want to solve this bug completely and in a good, permanent way, some refactoring is required (although I wouldn’t call it major refactoring). Otherwise, we would end up with a quick fix that might not be reliable in the long term. So I decided to address the issue properly rather than just patching the immediate symptom.

Also, once I started fixing this bug properly, it exposed a few related issues that needed to be addressed as well, which is why the scope of the changes grew.

Honestly, though, I don’t see a straightforward way to split this into separate PRs at this point, as the changes are quite interconnected.

If reviewing the PR would take significant time because of the amount of changes, I’m happy to leave it until after the {tern} release, when you have more time. In the meantime, I can just apply the fix in my package.

P.S. I didn’t change the names of any exported functions, only internal ones.

Please let me know what do you think.
Many thanks!

[munoztd0]

Hi @munoztd0 - would it be possible to run the scda tests against this PR?

here you go insightsengineering/scda.test#260

[danielinteractive]

Looks good to me, thanks @wwojciech. @Melkiades agree that this is a bigger PR than expected from the issue description, but there is almost no change to the user interface/behavior here (the p-value being NA instead of 1 for the empty data case being the only exception) so I would vouch for merging it nevertheless

[wwojciech]

Thank you @danielinteractive !

@Melkiades , @shajoezhu - would you be willing to merge this PR before the release?
This PR solves some important hidden bug (users can get wrong numbers without even a warning) so I think it would be nice to merge it before the release.
Please let me know what you think about this. Many thanks!

[Melkiades]

Thanks a lot for this, really nice work, and thanks for the patience during the review. Given how connected the fix is, no need to split it.

I pushed a small commit on top:

• uniroot_catch_na() checks for NA at the interval ends instead of matching the uniroot() error message. That message is translated in non-English R sessions (e.g. German or French), so the NA case was erroring there.
• NEWS moved under 0.9.12 and lists the user-facing changes.
• A small test checking that strata with only one group don't change the prop_diff_cmh() results.
• A few doc typos.

Approving, good to merge once CI is green.

[wwojciech]

Thanks a lot for this, really nice work, and thanks for the patience during the review. Given how connected the fix is, no need to split it.

I pushed a small commit on top:

• uniroot_catch_na() checks for NA at the interval ends instead of matching the uniroot() error message. That message is translated in non-English R sessions (e.g. German or French), so the NA case was erroring there.
• NEWS moved under 0.9.12 and lists the user-facing changes.
• A small test checking that strata with only one group don't change the prop_diff_cmh() results.
• A few doc typos.

Approving, good to merge once CI is green.

Thank you very much indeed @Melkiades - this is a great news!.
Looks great, I only did minor update to the help and added myself as contributor (I hope this is ok, otherwise, please let me know).

Many thanks again for helping with this!