Skip to content

1209: Skip subgroups with too few comparators in get_pairwise_comparisons()#1214

Draft
nikosbosse wants to merge 1 commit into
mainfrom
issue-1209-pairwise-small-subgroups
Draft

1209: Skip subgroups with too few comparators in get_pairwise_comparisons()#1214
nikosbosse wants to merge 1 commit into
mainfrom
issue-1209-pairwise-small-subgroups

Conversation

@nikosbosse

Copy link
Copy Markdown
Collaborator

Description

This PR closes #1209.

get_pairwise_comparisons() split scores into subgroups by by and then aborted as soon as any subgroup had fewer than two comparators, discarding results for all valid subgroups and without saying which subgroup was the problem. add_relative_skill() inherited the same behaviour.

With this PR, subgroups with fewer than two comparators are skipped with a warning that names them (as col=value pairs, e.g. "target_type=Deaths"), and results are returned for the remaining subgroups. add_relative_skill() fills NA for the skipped subgroups. If no subgroup has at least two comparators (including by = NULL with a single model), the previous error is kept unchanged, and the safeguard inside pairwise_comparison_one_group() remains for direct calls.

Tests were written first and failed on the unfixed code:

ERROR: 'test-pairwise_comparison.R:457:3' ----------
Error in `pairwise_comparison_one_group(...)`: ! There are not enough comparators to do any comparison
 7. └─scoringutils::get_pairwise_comparisons(scores_sub, by = "target_type")

ERROR: 'test-pairwise_comparison.R:494:3' ----------
Error in `pairwise_comparison_one_group(...)`: ! There are not enough comparators to do any comparison
 7. └─scoringutils::add_relative_skill(scores_sub, by = "target_type")

After the fix, the full test suite passes (0 failed, 915 passed). Note: the two vdiffr doppelganger snapshots for plot_pairwise_comparisons() fail locally on unmodified main as well (environment-dependent rendering) and are unrelated to this change.

Dev note: bug identified by an LLM audit (#1189); fix and tests implemented with LLM support, directed by @nikosbosse.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title as follows: issue-number: PR title
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • I have built the package locally and run rebuilt docs using roxygen2. (Rd changes limited to the three affected help pages to avoid unrelated formatting churn from a newer local roxygen2.)
  • My code follows the established coding standards and I have run lintr on the changed files; no new style issues were introduced by my changes.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

…odels (#1209)

get_pairwise_comparisons() split scores by 'by' and aborted as soon as
any subgroup had fewer than two comparators, discarding results for all
valid subgroups and without naming the offending subgroup. Subgroups
with fewer than two comparators are now skipped with a warning that
names them (as col=value pairs), and results are returned for the
remaining subgroups; add_relative_skill() fills NA for skipped
subgroups. If no subgroup has at least two comparators, the previous
error is kept, and the safeguard in pairwise_comparison_one_group()
stays in place for direct calls.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.17%. Comparing base (06ed56d) to head (45a789c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1214      +/-   ##
==========================================
+ Coverage   98.15%   98.17%   +0.01%     
==========================================
  Files          41       41              
  Lines        2225     2246      +21     
==========================================
+ Hits         2184     2205      +21     
  Misses         41       41              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nikosbosse

Copy link
Copy Markdown
Collaborator Author

Automated review (Claude Fable, directed by @nikosbosse):

Verdict: approve.

Verified independently in a clean worktree:

  • Fail-before/pass-after confirmed: with the branch's tests but R/ reverted to main, the two new tests fail with the original abort (! There are not enough comparators to do any comparison from pairwise_comparison_one_group()); with the branch's R/ restored, all tests pass.
  • Full test suite green on the branch (vdiffr snapshots skipped as on CRAN).
  • Edge cases probed beyond the PR's tests: multi-column by warning labels render in the maintainer-preferred format ("target_type=Deaths, location=DE"); exactly 2 comparators per group produces no spurious warning; two invalid groups out of two still errors (all-invalid branch); NA values in the by column are handled; values containing glue braces ({x}) do not break the cli warning.
  • Maintainer decisions from the brief are respected (warn+skip with named subgroups, col=value labels, add_relative_skill() fills NA, all-invalid still errors, internal safeguard untouched). NEWS entry accurate, docs regenerated, PR title format correct, scope tight.

Minor, non-blocking observations:

  1. A subgroup that is empty (e.g. unused factor level in a by column with split()'s drop = FALSE) is skipped with the label "target_type=NA" rather than the level name, because the label is taken from the first row of an empty table. On main this case hard-errored, so the PR is still a strict improvement; deriving labels from the split names would fix the cosmetics if desired.
  2. man/pairwise_comparison_one_group.Rd inherits the new "skipped with a warning" sentence in its by docs, but that internal function itself still errors rather than skips. Inherited-param side effect; cosmetic only.

@nikosbosse
nikosbosse marked this pull request as draft July 18, 2026 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_pairwise_comparisons() aborts entirely when any subgroup has too few models

1 participant