Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds new contrast functions for log ratios and log odds ratios to improve statistical analysis capabilities. The implementation includes appropriate warnings to guide users toward better variance estimation practices and to alert them when odds ratios may not be appropriate for their data.
- Added
log_ratio,log_odds_ratio, andodds_ratiocontrast functions with corresponding Jacobian methods - Implemented warning system for ratio/odds ratio usage and invalid probability ranges
- Added comprehensive tests for the new functionality and warning behaviors
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| R/robincar-contrast.R | Implements new contrast functions, Jacobian methods, and warning logic |
| tests/testthat/test-contrast.R | Adds test coverage for warnings and validates log odds ratio calculations |
|
|
||
| #' @exportS3Method | ||
| jacobian.ODDSRATIO <- function(settings, est){ | ||
| col1 <- -1 / est[1] ** 2 * est[-1] / (1-est[-1]) |
There was a problem hiding this comment.
The complex mathematical expression should be broken down or commented for clarity. Consider splitting into multiple lines or adding a comment explaining the mathematical formula being implemented.
| col1 <- -1 / est[1] ** 2 * est[-1] / (1-est[-1]) | |
| # Compute the first column of the Jacobian for the odds ratio transformation. | |
| # Formula: -1 / (est[1]^2) * est[-1] / (1 - est[-1]) | |
| base_sq <- est[1]^2 | |
| odds_cont <- est[-1] / (1 - est[-1]) | |
| col1 <- -1 / base_sq * odds_cont |
| #' @exportS3Method | ||
| jacobian.ODDSRATIO <- function(settings, est){ | ||
| col1 <- -1 / est[1] ** 2 * est[-1] / (1-est[-1]) | ||
| mat1 <- diag((1-est[-1]) / est[-1] / (1-est[1]) ** 2, length(est)-1) |
There was a problem hiding this comment.
The complex mathematical expression should be broken down or commented for clarity. Consider splitting into multiple lines or adding a comment explaining the mathematical formula being implemented.
| mat1 <- diag((1-est[-1]) / est[-1] / (1-est[1]) ** 2, length(est)-1) | |
| # mat1 is the derivative of the odds ratio with respect to each comparison group. | |
| # It is calculated as: (1 - est[-1]) / (est[-1] * (1 - est[1])^2) | |
| numer <- (1 - est[-1]) | |
| denom <- est[-1] * (1 - est[1])^2 | |
| mat1 <- diag(numer / denom, length(est)-1) |
| if(class(settings)[2] %in% c("ODDSRATIO", "RATIO")){ | ||
| warning("Using a ratio or odds ratio is not recommended. | ||
| Consider using log_ratio or log_odds_ratio for better | ||
| performance of the variance estimator.") |
There was a problem hiding this comment.
The warning message has inconsistent indentation and formatting. The multi-line string should be properly formatted, either as a single line or with consistent indentation.
| performance of the variance estimator.") | |
| warning("Using a ratio or odds ratio is not recommended. Consider using log_ratio or log_odds_ratio for better performance of the variance estimator.") |
|
|
||
| warning_txt <- "Using a ratio or odds ratio is not recommended. | ||
| Consider using log_ratio or log_odds_ratio for better | ||
| performance of the variance estimator." |
There was a problem hiding this comment.
The warning text has inconsistent formatting with extra whitespace and indentation that doesn't match the actual warning message in the source code. This could cause test failures if the formatting doesn't exactly match.
| performance of the variance estimator." | |
| warning_txt <- "Using a ratio or odds ratio is not recommended. Consider using log_ratio or log_odds_ratio for better performance of the variance estimator." |
| test_that("Compare log odds ratio SEs to GLM", { | ||
| # We want these to give warnings when someone | ||
| # specifies that they want a log odds ratio. | ||
|
|
There was a problem hiding this comment.
The comment is incorrect for this test. This test doesn't check for warnings - it compares log odds ratio estimates to GLM output. The comment appears to be copied from the previous test.
| # Compare log odds ratio estimates from robincar_glm to standard GLM output. |
1 participant
log_ratio,log_odds_ratio, andodds_ratioas options for the built-in contrasts.odds_ratioorratiosaying they should use the log options instead because they give better performance with variance estimation.odds_ratioorlog_odds_ratioand their estimates are outside of the 0-1 range since this would mean they probably didn't want something on the odds scale, since they clearly don't have a probability.