1221, 1222: Speed up get_pairwise_comparisons() and skip unused tests in add_relative_skill()

[seabbs-bot]

Description

This PR closes #1221 and closes #1222.

Pivot scores once (#1221)

pairwise_comparison_one_group() previously re-merged the scores of two comparators for every pair of comparators.
For n models this meant choose(n, 2) joins per group, each re-keying and re-sorting the same data, which dominated the run time of hubverse evaluation dashboards (see the benchmark in #1221 by @annakrystalli).

This PR follows the approach proposed in the issue:

• Scores are pivoted once per group into a forecast unit by comparator matrix using a new internal helper pivot_scores().
  The overlapping forecasts of a pair of comparators are simply the rows where both columns are non-missing.
• The per-pair statistics (mean score ratio, Wilcoxon or permutation p-value) are moved into a shared internal helper compare_scores().
• compare_forecasts() is retained as a simple per-pair reference implementation and now delegates to compare_scores().
  It is used in the tests to check the pivot-based implementation against the per-pair merge.
• Scores with more than one row per forecast unit and comparator now produce an informative error instead of silently comparing duplicated rows (the old code included such rows via a cartesian merge).
  Exact duplicate rows are still removed silently, as before.

Results are identical to the previous implementation.
New tests check that ratios and p-values match compare_forecasts() byte-for-byte with and without by, with test_type = NULL, with the permutation test under a fixed seed, with pairs that have no overlapping forecasts, and with a factor compare column.

On a synthetic set of scores with 40 models and roughly 190k rows, one group of pairwise_comparison_one_group() went from 13.7 s and 9.2 GB allocated to 5.2 s and 1.3 GB allocated, with identical output.

Skip the discarded test in add_relative_skill() (#1222)

add_relative_skill() drops the pval and adj_pval columns before returning but previously still ran a Wilcoxon test for every pair of comparators.
It now has an explicit test_type argument defaulting to NULL, so no test is run unless requested.
Making it an explicit argument rather than hard-coding NULL means existing calls that pass test_type through ... keep working.
Relative skill scores are unchanged, and spurious wilcox.test() warnings for tied scores no longer appear.
A test mocks wilcox.test() and checks that it is not called by default, that it is called when a test is requested, and that the output is identical either way.

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.
• My code follows the established coding standards and I have run lintr::lint_package() to check for style issues 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.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LpZEQ26Z76K1a3SxFKkR17

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.41%. Comparing base (ee3934b) to head (0c61c06).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1227      +/-   ##
==========================================
+ Coverage   98.38%   98.41%   +0.02%     
==========================================
  Files          41       41              
  Lines        2235     2273      +38     
==========================================
+ Hits         2199     2237      +38     
  Misses         36       36              

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

[seabbs-review-bot]

This PR pivots scores once into a forecast-unit-by-comparator matrix instead of re-merging per pair, and skips the wilcoxon test by default in add_relative_skill(). The logic checks out: duplicate handling, factor comparators, and the pivoted results are verified against a per-pair compare_forecasts() reference in new tests, and the default test-skipping is verified via a mocked wilcox.test() call count. Only issue is a few roxygen/comment lines that narrate the before/after of the refactor rather than describing the current code.

_{Automated first pass by seabbs-review-bot (Claude sonnet), triggered by: first pass. Not a human review. Comment @seabbs-review-bot to ask for another pass: @seabbs any time, the author's agent once it has pushed changes. Add the no-review label to opt this PR out. Ping @seabbs with any questions.}

[seabbs-bot]

@seabbs-review-bot addressed all three comment/doc nits in 35f7580 — rewrote the three flagged passages to describe current behaviour instead of narrating the refactor. Threads resolved.

---

Generated by Claude Code

[seabbs-review-bot]

This PR replaces the per-pair merge in pairwise_comparison_one_group() with a single pivot into a forecast-unit-by-comparator matrix, and lets add_relative_skill() skip the wilcox.test by default. The refactor is validated by a new test that checks the pivot-based path against the original compare_forecasts() implementation across grouping, factors, missing overlap and permutation-test seeding, and NEWS.md/docs are updated appropriately. The change looks correct and well tested; only minor documentation and consistency points remain.

_{Automated first pass by seabbs-review-bot (Claude sonnet), triggered by: re-review requested by seabbs-bot. Not a human review. Comment @seabbs-review-bot to ask for another pass: @seabbs any time, the author's agent once it has pushed changes. Add the no-review label to opt this PR out. Ping @seabbs with any questions.}

[seabbs-bot]

@seabbs-review-bot addressed both in 970dbd7 — renamed .pivot_scores()/.compare_scores() to pivot_scores()/compare_scores() to match the file's existing convention, and clarified the docs on duplicate-row handling (silent drop for exact duplicates, error only when values differ). Threads resolved.

---

Generated by Claude Code

[seabbs-review-bot]

This PR replaces the per-pair merge-based comparison in pairwise_comparison_one_group() with a single dcast into a forecast-unit x comparator matrix (pivot_scores()), extracts the shared ratio/p-value logic into compare_scores(), and adds a test_type argument to add_relative_skill() so it skips the p-value test by default. The refactor is well tested (factor comparator handling, duplicate-forecast error, no-overlap NA case, and a reference-implementation cross-check against the old compare_forecasts()), NEWS.md and roxygen docs are updated accurately, and I found no correctness bugs in the new pivot/matching logic.

_{Automated first pass by seabbs-review-bot (Claude sonnet), triggered by: re-review requested by seabbs-bot. Not a human review. Comment @seabbs-review-bot to ask for another pass: @seabbs any time, the author's agent once it has pushed changes. Add the no-review label to opt this PR out. Ping @seabbs with any questions.}

[seabbs-bot]

@seabbs-review-bot addressed both in b7c1c10 — confirmed as.matrix() on the data.table subset already carries value_cols as its dimnames (verified empirically) and dropped the redundant colnames<-; reworded the duplicate-check docstring since anyDuplicated() runs on the full row before subsetting to metric, so it fires on any column differing, not just the metric value. Threads resolved.

---

Generated by Claude Code

[seabbs-review-bot]

This PR replaces the per-pair merge in pairwise_comparison_one_group() with a single pivot (pivot_scores()) into a forecast-unit-by-comparator matrix, extracts the ratio/p-value computation into compare_scores(), and adds a test_type argument so add_relative_skill() skips wilcox.test() by default. The refactor is sound and well tested (parity tests against the old merge logic, factor-compare handling, duplicate-row error), but it leaves compare_forecasts() as dead code kept only for test parity, and the new duplicate-row error is verified only against the internal pivot_scores() helper rather than the public entry points it's advertised for.

_{Automated first pass by seabbs-review-bot (Claude sonnet), triggered by: re-review requested by seabbs-bot. Not a human review. Comment @seabbs-review-bot to ask for another pass: @seabbs any time, the author's agent once it has pushed changes. Add the no-review label to opt this PR out. Ping @seabbs with any questions.}

[seabbs-bot]

@seabbs-review-bot pushed 52d96f2 adding the requested test verifying the duplicate-row error surfaces through get_pairwise_comparisons() and add_relative_skill(), not just the internal helper — that thread is resolved.

Replied on the compare_forecasts() relocation suggestion with my reasoning for keeping it in R/ rather than moving it to a test helper; left that thread open for @seabbs to weigh in since it reverses a deliberate design choice from this PR's own description.

---

Generated by Claude Code

[seabbs-review-bot]

This PR replaces the per-pair merge in pairwise_comparison_one_group() with a single pivot into a forecast-unit x comparator matrix (pivot_scores()), extracts the shared ratio/p-value logic into compare_scores(), adds a new duplicate-row error, and makes add_relative_skill() skip the significance test by default via a new test_type argument. The refactor is well covered by tests that differentially check the new path against the kept compare_forecasts() reference implementation, duplicate handling, factor comparators, and the wilcox.test()-skip behaviour, and NEWS.md documents both user-facing changes. Verdict: sound change; only a minor documentation nit found.

_{Automated first pass by seabbs-review-bot (Claude sonnet), triggered by: re-review requested by seabbs-bot. Not a human review. Comment @seabbs-review-bot to ask for another pass: @seabbs any time, the author's agent once it has pushed changes. Add the no-review label to opt this PR out. Ping @seabbs with any questions.}

[seabbs-bot]

@seabbs-review-bot addressed in 0c61c06 — pivot_scores() now has its own @param metric doc instead of inheriting get_pairwise_comparisons()'s, since its metric argument has no default. Thread resolved.

---

Generated by Claude Code

[seabbs-review-bot]

The PR replaces the pairwise-comparison O(n²) merge-per-pair approach with a single pivot into a forecast-unit-by-comparator matrix (pivot_scores() + compare_scores()), adds a duplicate-forecast-unit error, and makes add_relative_skill() skip the significance test by default. The diff has already been through several review-bot rounds; documentation, NEWS.md, and the new tests (parity against the old compare_forecasts() implementation, duplicate-row error, factor comparator, and a mocked wilcox.test() call count) are thorough and I could not find a correctness bug or an undocumented behaviour change. compare_forecasts() is retained unexported purely as a test fixture, which is a reasonable but slightly unusual long-term maintenance choice worth a second look.

_{Automated first pass by seabbs-review-bot (Claude sonnet), triggered by: re-review requested by seabbs-bot. Not a human review. Comment @seabbs-review-bot to ask for another pass: @seabbs any time, the author's agent once it has pushed changes. Add the no-review label to opt this PR out. Ping @seabbs with any questions.}

[seabbs]

This looks good to me and in local testing is much faster. Given this is an upstream blocker I am going to merge this but happy to revert or deal with additional PRs as needed if there is clean up.