1210: Fix plot_correlations() heatmap misalignment with non-default metrics order

[nikosbosse]

Description

This PR closes #1210.

plot_correlations() produced a misaligned heatmap whenever the input was created with get_correlations(scores, metrics = ...) in a non-default metric order (reversed or a reordered subset): cell values were attached to the wrong metric pairs and the diagonal was not 1 (with reversed default metrics, 30 of 36 plotted cells were wrong).

Cause: get_correlations() selected score columns with a logical .SDcols, so its rows/columns came back in data-column order while the stored metrics attribute kept the user-supplied order. plot_correlations() then reordered only the columns to attribute order and labelled the rows positionally with metrics.

Fix (as approved in #1210):

• get_correlations() now selects columns with a character .SDcols = metrics, so rows, columns, the metric column, and the metrics attribute all share the requested order (the originally intended behaviour).
• plot_correlations() now aligns rows explicitly via the metric column (rownames(cor_mat) <- correlations$metric; cor_mat[metrics, , drop = FALSE]) instead of relying on positional order, so it also plots objects created by the pre-fix get_correlations() correctly. The validation check was moved before matrix construction; the error message for non-correlation input is unchanged, and the default-order vdiffr snapshot is unchanged.

New tests (reversed order and reordered subset) fail on unfixed code with, e.g.:

FAILURE: 'test-get-correlations.R:46:3'
Expected `diag(as.matrix(correlations[, .SD, .SDcols = m]))` to equal `rep(1, length(m))`.
[2] 0.196848 - 1.000000 [2]
[3] 0.163029 - 1.000000 [3]
...
FAILURE: 'test-get-correlations.R:83:3'
Expected `all(pd[as.character(metric) == as.character(variable)]$value == 1)` to be TRUE.

After the fix, every plotted cell matches the true correlation matrix and the full test suite passes (0 failed, 914 passed).

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. (N/A — no user-facing documentation changes needed; roxygen docs unchanged)
• I have built the package locally and run rebuilt docs using roxygen2. (N/A — no roxygen comments changed, so no man/ files needed rebuilding)
• My code follows the established coding standards and I have run lintr on the changed files 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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.16%. Comparing base (06ed56d) to head (4ad33f7).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1215   +/-   ##
=======================================
  Coverage   98.15%   98.16%           
=======================================
  Files          41       41           
  Lines        2225     2231    +6     
=======================================
+ Hits         2184     2190    +6     
  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]

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

Verdict: approve.

Checks performed:

• Fail-before/pass-after verified independently: with the branch tests and R/ reverted to origin/main, the new tests fail with 7 failures for exactly the stated reasons (data-order columns/rows, non-1 diagonal, misaligned plotted cells); on the PR commit they pass.
• Full test suite green: 0 failed, 914 passed (matches the PR body).
• Edge cases reproduced independently, all correct: (a) backward compatibility confirmed — an object built by the pre-fix get_correlations() (rows in data order, attribute in reversed user order) now plots with an all-1 diagonal and every cell matching the true correlation matrix; (b) single-metric input works; (c) NA correlations (constant score column) are handled as before (NA cells dropped by na.omit); (d) shuffled 4-metric subset matches stats::cor truth; (e) default order is byte-identical to truth, so the existing vdiffr snapshot is legitimately unchanged.
• Matches the approved decisions in plot_correlations() misaligns the heatmap with non-default metrics order #1210; scope is tight (NEWS + R/get-correlations.R + tests); NEWS entry accurate; PR title format correct; no lint issues in the changed code (the file's pre-existing standalone-lint object_usage false positives aside).

Non-blocking observations:

1. Test coverage gap for the plot-side alignment: the explicit row alignment in plot_correlations() (part 2, the backward-compat behaviour claimed in the PR body) has no dedicated test — if it were later reverted while the get_correlations() fix stayed, all tests would still pass. A small test feeding plot_correlations() a row-scrambled correlations object would pin it down.
2. Degenerate input metrics = c("wis", "wis", "bias") now yields duplicated columns and a wis.1 metric label. The old code also misbehaved on duplicates (errored at plot time), so this is not a regression, but a checkmate uniqueness assertion would be cheap.

[nikosbosse]

Follow-up (Claude Fable, directed by @nikosbosse): both non-blocking review observations are addressed in 4ad33f7.

1. Plot-side alignment test: added a dedicated test that feeds plot_correlations() a row-scrambled correlations object (rows and metric column in data order, metrics attribute in reversed order — simulating an object created by the pre-fix get_correlations()) and asserts via the ggplot data that all diagonal cells are 1 and every cell matches the true correlation matrix. Verified that this test fails (2 failures: diagonal and cell-match assertions) when the explicit row-alignment block in plot_correlations() is temporarily reverted to positional labelling while the get_correlations() fix stays, and passes with the fix in place.
2. Duplicate metrics: get_correlations() now asserts assert_character(metrics, unique = TRUE) (after the existing subset check, preserving the error message for missing-attribute input), so metrics = c("wis", "wis", "bias") errors cleanly; covered by an expect_error test.

Test file: 21 pass, 0 fail. Full suite: 0 failed, 0 errors, 917 passed. lintr on both changed files: no new lints (only the file's pre-existing standalone object_usage false positives, now also flagging the added assert_character call for the same NAMESPACE-visibility reason).