Skip to content

Conversation

@mccarthy-m-g
Copy link
Collaborator

@mccarthy-m-g mccarthy-m-g commented Oct 23, 2025

This PR adds an (optional) accuracy metric to the results stats summary of run_eval() (#30).

The main challenge for this one was defining the API, since unlike other error metrics, the user needs to define the absolute and relative error margins for whether a prediction is considered accurate or not. I opted to only calculate accuracy if the user has supplied the absolute and relative error margins (via a new .stats_summ_options argument and corresponding stats_summ_options() function). This lets us avoid defining universally suitable defaults, which is probably difficult to do appropriately.

The API looks like this:

run_eval(..., .stats_summ_options = stats_summ_options(acc_error_abs = 0.5, acc_error_rel = 0.25)

@mccarthy-m-g mccarthy-m-g added the enhancement New feature or request label Oct 23, 2025
@mccarthy-m-g mccarthy-m-g linked an issue Oct 23, 2025 that may be closed by this pull request
@roninsightrx
Copy link
Contributor

I opted to only calculate accuracy if the user has supplied the absolute and relative error margins

Agree with this approach, otherwise there is too much chance of users just relying on what is default.

@mccarthy-m-g
Copy link
Collaborator Author

One Q for printing: Right now we're printing accuracy with NAs for each type when it isn't being calculated. Do you like that, or should we remove the accuracy column from printing when it isn't calculated?

To hide it, we just need to add a select statement to print.mipdeval_results_stats_summ(), like:

tibble(x = rep(NA, times = 5), y = 1:5) |> select(where(\(.x) all(!is.na(.x))))
#> # A tibble: 5 × 1
#>       y
#>   <int>
#> 1     1
#> 2     2
#> 3     3
#> 4     4
#> 5     5

nrmse = c(0.411, 0.232, 0.199, 0.121, 0.486, 0.232),
mpe = c(-0.339, -0.004, -0.045, -0.002, -0.415, -0.004),
mape = c(0.445, 0.246, 0.166, 0.118, 0.558, 0.246),
accuracy = c(NA_real_, NA_real_, NA_real_, NA_real_, NA_real_, NA_real_)
Copy link
Contributor

Choose a reason for hiding this comment

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

would include at least one end-to-end test for run_eval() where we do calculate it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added here: 070a7bb

@roninsightrx roninsightrx self-requested a review October 29, 2025 22:48
Copy link
Contributor

@roninsightrx roninsightrx left a comment

Choose a reason for hiding this comment

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

lgtm!

@mccarthy-m-g mccarthy-m-g merged commit 834ef50 into main Nov 10, 2025
2 checks passed
@mccarthy-m-g mccarthy-m-g deleted the stat-accuracy branch November 10, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add accuracy metric to results stats summary

3 participants