Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions modules/benchmark/R/metric_R2.R
Original file line number Diff line number Diff line change
@@ -1,23 +1,39 @@
##' @name metric_R2
##' @title Coefficient of Determination (R2)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

title still says "Coefficient of Determination" but formula in @details squared pearson correlation; formula in @details is correct.
change title

##' @export
##' @param metric_dat dataframe
##' @param metric_dat dataframe with columns \code{model} and \code{obvs}
##' @param ... ignored
##'
##'
##' @details
##' Computes R-squared using the correlation-based formula:
##' \eqn{R^2 = \left(\frac{\sum(obs - \bar{obs})(mod - \bar{mod})}
##' {\sqrt{\sum(obs - \bar{obs})^2} \cdot \sqrt{\sum(mod - \bar{mod})^2}}\right)^2}
##'
##' If this formula returns \code{NA} (e.g. when model output is constant
##' across all observations), the function silently falls back to an
##' \code{lm()}-based R-squared via \code{summary(lm())$r.squared}.
##' This fallback may produce unreliable results and triggers a warning
Comment on lines +12 to +15

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here "silently falls back" and next sentence says it triggers warning. can't both be true
drop "silently"; rest of the paragraph correctly describes the warning behavior

##' from \code{stats::summary.lm}: "essentially perfect fit: summary may
##' be unreliable". Consider checking for constant model output before
##' calling this function.
##'
##' @author Betsy Cowdery

metric_R2 <- function(metric_dat, ...) {
PEcAn.logger::logger.info("Metric: Coefficient of Determination (R2)")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also comment here -- https://github.com/PecanProject/pecan/pull/3888/changes#r3397678145

Suggested change
PEcAn.logger::logger.info("Metric: Coefficient of Determination (R2)")
PEcAn.logger::logger.info("Metric: Squared Pearson Correlation (R2)")

numer <- sum((metric_dat$obvs - mean(metric_dat$obvs)) * (metric_dat$model - mean(metric_dat$model)))
denom <- sqrt(sum((metric_dat$obvs - mean(metric_dat$obvs)) ^ 2)) * sqrt(sum((metric_dat$model - mean(metric_dat$model)) ^ 2))

out <- (numer / denom) ^ 2

# If correlation formula returns NA (e.g. constant model output),
# fall back to lm()-based R-squared. Note: this fallback may trigger
# "essentially perfect fit" warning from stats::summary.lm and
# produce unreliable results in edge cases.
if(is.na(out)){
fit <- stats::lm(metric_dat$model ~ metric_dat$obvs)
out <- summary(fit)$r.squared
}

return(out)

} # metric_R2
} # metric_R2
17 changes: 15 additions & 2 deletions modules/benchmark/man/metric_R2.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

49 changes: 49 additions & 0 deletions modules/benchmark/tests/testthat/test-metrics.R

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NA in model triggers lm() fallback, which by default just drops NA row via na.action = na.omit and fits perfect line through whatever's left. so single NA in model series silently returns R2 = 1. no test pins this. wondering if the ryt move here is to handle NAs same way metric_cor and metric_PPMC do, squaring that gives same r2 and lm fallback can go away entirely, since cor() already returns NA for degenerate constant input case; so constant input case returns NA, instead of NaN

something like:

metric_R2 <- function(metric_dat, ...) {
  PEcAn.logger::logger.info("Metric: Squared Pearson Correlation (R²)")
  stats::cor(metric_dat$model, metric_dat$obvs, use = "pairwise.complete.obs")^2
}

heads up: if this lands, the last test needs to update too , constant model case would return NA (with "standard deviation is zero" warning) instead of NaN (with "essentially perfect fit" warning)

Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# at the top of test-metrics.R, add this:
if (!requireNamespace("PEcAn.logger", quietly = TRUE)) {
PEcAn.logger <- new.env()
PEcAn.logger$logger.info <- function(...) invisible(NULL)
Comment on lines +3 to +4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what's happening here and what it's trying to accomplish? It looks like you may be trying to mask the logger functions, which I do not think will work as expected (they live in their own namespace) and also isn't generally a good idea.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infotroph I focused my review on the metric functions and the R-squared fallback documentation (which align perfectly with the Phase 2 goals of my GSoC workplan) and I admittedly glossed over that mocking workaround at the top of the test file.
Looking at the PR description, it seems the author added that block to bypass a local environment issue where they couldn't install PEcAn.logger. You are absolutely right that we shouldn't be trying to mask the namespace like this, especially since the package will be available in the standard CI/testing environment anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tanmaydimriGSOC could you go ahead and remove lines 1-5 in test-metrics.R? The GitHub CI will have the necessary dependencies installed so the tests will run fine without that workaround. Once that is removed the core logic of this PR still looks good to me!!

}

test_that("metric_RMSE returns 0 for perfect predictions", {
dat <- data.frame(model = c(1, 2, 3), obvs = c(1, 2, 3))
expect_equal(metric_RMSE(dat), 0)
})

test_that("metric_RMSE handles NA values", {
dat <- data.frame(model = c(1, NA, 3), obvs = c(1, 2, 3))
expect_true(is.numeric(metric_RMSE(dat)))
})
Comment on lines +12 to +15

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is.numeric() passes on NA, NaN, Inf, and 0 indifferently, this test doesn't actually constrain the return value. since metric_RMSE uses na.rm = TRUE, expected value here is 0. worth checking that directly

Suggested change
test_that("metric_RMSE handles NA values", {
dat <- data.frame(model = c(1, NA, 3), obvs = c(1, 2, 3))
expect_true(is.numeric(metric_RMSE(dat)))
})
test_that("metric_RMSE handles NA values", {
dat <- data.frame(model = c(1, NA, 3), obvs = c(1, 2, 3))
expect_equal(metric_RMSE(dat), 0)
})


test_that("metric_RMSE returns numeric", {
dat <- data.frame(model = c(2, 4), obvs = c(1, 3))
expect_equal(metric_RMSE(dat), 1)
})

test_that("metric_MAE returns 0 for perfect predictions", {
dat <- data.frame(model = c(1, 2, 3), obvs = c(1, 2, 3))
expect_equal(metric_MAE(dat), 0)
})

test_that("metric_MAE returns correct value", {
dat <- data.frame(model = c(3, 3), obvs = c(1, 1))
expect_equal(metric_MAE(dat), 2)
})

test_that("metric_cor returns 1 for perfect linear relationship", {
dat <- data.frame(model = c(1, 2, 3), obvs = c(1, 2, 3))
expect_equal(metric_cor(dat), 1)
})

test_that("metric_R2 returns 1 for perfect predictions", {
dat <- data.frame(model = c(1, 2, 3), obvs = c(1, 2, 3))
expect_equal(metric_R2(dat), 1)
})

test_that("metric_R2 silent NA fallback produces valid output", {
dat <- data.frame(model = c(2, 2, 2), obvs = c(1, 2, 3))
expect_warning(
result <- metric_R2(dat),
"essentially perfect fit"
)
expect_true(is.numeric(result))
})
Comment on lines +42 to +49

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test name says "silent NA fallback" but the input has no NAs, constant model triggers fallback via 0/0 returning NaN. assertion is.numeric(result) also passes on NaN, which is what the function actually returns here. worth tightening both

Loading