Add Mantel-Fleiss criterion function mantel_fleiss_crit().

[wwojciech]

Fixes #1512

[wwojciech]

@danielinteractive - I was wondering whether I should introduce an na.rm = FALSE parameter and allow missing values. Currently, I want to be consistent with the other tern prop_*() functions, which expect no missing values.

Do you think having an na.rm argument and allowing missing values would be useful?

[danielinteractive]

Thanks @wwojciech , please see few comments below - otherwise looks nice.
I don't think we need na.rm argument when the other functions which are used together with this don't have it.

[wwojciech]

@danielinteractive - I am now thinking that it would probably be better to change the UI to tbl (contingency table), instead of grp, rsp, and strata, and introduce a separate helper to reliably build tbl from these three vectors.

Most (if not all) of the prop_* test functions (prop_cmh(), prop_fisher(), ...) already require tbl, so I would need to create such a table anyway, since after checking the MF criterion I need to run the test.

[danielinteractive]

@wwojciech yeah starting from the table is probably a good idea

[wwojciech]

Thank you @danielinteractive for your review. I’ve updated the code to address all of your suggestions and changed the UI as discussed. Please review the PR again when you have a chance. Thanks!

[wwojciech]

@danielinteractive - do you think this below little helper should go to {tern} or to {junco}? I would probably say {tern}...

#' @param rsp (`logical`)\cr
#'   A logical vector indicating whether each observation is a responder.
#' @param grp (`factor`)\cr
#'   A factor assigning observations to one of two groups (e.g., reference
#'   and treatment). It must have exactly two levels.
#' @param strata (`factor`)\cr
#'   Factor defining the stratification variable. Each unique
#'   stratum defines a separate 2 x 2 contingency table.
safe_2x2_strat_table <- function(rsp, grp, strata) {
  checkmate::assert_logical(rsp, any.missing = FALSE)
  checkmate::assert_factor(grp, len = length(rsp), any.missing = FALSE, n.levels = 2)
  checkmate::assert_factor(strata, len = length(rsp), any.missing = FALSE)

  # Make rsp a factor to handle cases with only TRUE or only FALSE.
  rsp <- factor(rsp, levels = c("TRUE", "FALSE"))

  table(grp, rsp, strata)
}

[danielinteractive]

Thanks @wwojciech yeah to tern because there it can be used in multiple places

[wwojciech]

Thank you @danielinteractive .
@Melkiades , @shajoezhu - can you pl. merge this PR if you have no objections to the code? Thank you!

[danielinteractive]

@wwojciech do you still need to write this CLA comment thing?

[wwojciech]

@wwojciech do you still need to write this CLA comment thing?

Thanks @danielinteractive , I am not sure about it? Do I have to do this? I thought this is will be disabled - see this PR #1508 (my previous PRs were merged with the CLA check failing). What do you think?

[danielinteractive]

@wwojciech ah yes now that tern is in pharmaverse this is not needed anymore indeed. Thanks!

[danielinteractive]

Looks good to me @wwojciech . @shajoezhu @Melkiades can we merge ?

[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.

Thanks!

[Melkiades]

The function itself is fine, math checks out and tests are solid. My blocker is the same as before: it is exported but not used anywhere in tern. s_test_proportion_diff() still picks CMH vs Fisher purely from the method arg, it never calls this. Moving it to prop_diff.R does not fix that, the problem is integration not location.

So a decision is needed: is this an internal helper that drives method selection, or a public function users call themselves?

• internal: drop @export, use the h_ prefix, and call it from s_test_proportion_diff(). This also settles the value/crit attribute thread, no need for it if it is internal.
• public: keep it exported but the examples must show the full tern workflow that builds the tbl, not just table() on random data. Otherwise users have no way to know when or how to use it.

Until one of those is done I do not think this should go in.

[wwojciech]

Thank you @Melkiades for your review and valuable comments. I completely agree with your concerns.

Here are my thoughts regarding them:

1. I think this function should remain exported. Users of {tern} (including the {junco} package) often write custom analysis/content functions using {tern}'s proportion-related functions. In these cases, the decision about which approach to use (e.g., CMH or exact) can be based on the Mantel-Fleiss criterion. Therefore, I think this function needs to be available to {tern} users for this purpose.

2. I will prepare some full {tern} workflow examples showing how this function can be used when writing custom analysis functions. These examples will demonstrate how to use the Mantel-Fleiss criterion to inform the choice of method.

3. I also think it would be useful to open a separate discussion/issue to discuss whether new or existing proportion-related analysis/content functions that automatically select the appropriate test based on the Mantel-Fleiss criterion (e.g., CMH vs. exact) should be added to or updated in {tern}. I opened issue Review test selection for proportion-related analyses based on the Mantel-Fleiss criterion #1518 for this.

Many thanks again!

[Melkiades]

Thanks for this. The stats side is clear and I am happy to have the criterion available, the issue is purely how it connects into tern.

Right now mantel_fleiss_crit() is exported but nothing calls it, no table or analysis function uses it, and no test reaches it through any tern mechanism. The documentation describes it in isolation, unconnected to anything in the package.

Two options:

1. Wire it into the prop diff test path and add a test plus examples that show it in the context where it is actually used, with @Seealso pointing to those functions.
2. If it is a standalone utility, say so, move it to its own utils file, and document it as such. If it really is meant to stand alone, a short vignette showcasing when and how to use it would make it discoverable.

Naming should state the action too: check_mantel_fleiss() reads better than mantel_fleiss_crit().

I would rather we solve this here than defer to a follow-up issue. We are trying not to leave orphan functions that get forgotten and unmaintained, so let us land it properly connected in this PR.

[wwojciech]

Hi @Melkiades

Thanks for the detailed feedback. I understand the concern about not introducing an exported function that is disconnected from the rest of {tern}, and I agree that, from a package-maintenance perspective, it would be better to have the criterion properly integrated into an existing analysis path.

However, I think that integration is beyond the scope of what I can reasonably take on in this PR. I am not a tern developer (anymore), and while I was happy to contribute the Mantel-Fleiss criterion and make the necessary minimal changes around it, I don't have the time to refactor the relevant analysis/test paths and add the broader integration, examples, and tests you are asking for.

I have already invested quite a bit of time in this PR, so rather than extending its scope substantially, I think the best solution is to close it and implement the Mantel-Fleiss criterion in the package I support, where I can maintain and use it directly.

I appreciate the review and understand the rationale behind the request. I just don't think I can take on the additional {tern} integration work at this point. Thanks again for taking the time to review the contribution.

@danielinteractive - FYI.