fix: address Codex review items for 3.5.1

[ehrlinger]

Rolls the Codex review items into 3.5.1. Six items; two real defects, the rest documentation and test-harness consistency.

Defects fixed

gg_roc(rfsrc_fit, which_outcome = 0) returned an unusable object. The help page has always documented 0 as the numeric spelling of "all", but only the string was normalized. 0 fell through to predicted[, 0] — a legal zero-column subset rather than an error — so the threshold sweep ran on empty input and returned a two-row frame whose columns were X1/X2/pct, with no sens/spec at all. calc_auc() then failed four calls later. Both spellings now take the same route: warn, fall back to class 1. The macro-average that will replace the fallback stays under #72.

Worth noting 99, -1, and "setosa" all error cleanly on this path. Zero is the one index R treats as "select nothing" rather than "out of range", which is why it alone produced a plausible-looking wrong answer instead of a stack trace.

gg_partial_rfsrc() had no upfront type check. It read $xvar and $xvar.names before validating class, so gg_partial_rfsrc(list(), xvar.names = "x") died with base R's argument is of length zero. It now raises a package error matching the style already used by gg_error(), gg_vimp(), gg_variable() and gg_rfsrc().

Both are covered by new regression tests.

Documentation

The three ROC entry points still disagree about what "all classes" means — gg_roc() on a randomForest fit macro-averages, gg_roc() on an rfsrc fit falls back to class 1, and a direct plot.gg_roc() call on a raw multi-class forest overlays one curve per class. Runtime behaviour is unchanged here (verified identical: 200/60/78 rows across the three paths). What changed is that ?gg_roc and ?plot.gg_roc now state the divergence instead of implying the paths agree, and plot.gg_roc no longer claims it "calls gg_roc() for you" when it does not use gg_roc()'s defaults. Reconciling them is minor-version work, tracked under #72.

Correction in a0171d9 (Copilot review). The first commit's wording said plot() on a raw forest reaches plot.gg_roc(). It does not — randomForestSRC and randomForest both register their own plot methods, so plot(rfsrc_fit) dispatches to plot.rfsrc() and returns NULL. That branch of plot.gg_roc() is reachable only by naming the method outright. The pre-existing line "Hand it a forest and gg_roc() is called for you" had the same defect, so this correction predates the PR. It also settles the open question the original review raised about whether that path is public behaviour: normal dispatch never reaches it.

Found while verifying: ?gg_roc also advertised character class names, but the rfsrc path errors with subscript out of bounds on them — only the randomForest method accepts them. That claim is corrected.

The README function table listed 9 of ~29 exports. It now covers the varPro, SHAP, ROC and survival helpers too, grouped by task rather than as one flat list, and the changelog runs to 3.5.1 instead of stopping at 3.4.0.

The pbc examples on ?gg_error, ?plot.gg_error, ?gg_vimp and ?plot.gg_rfsrc lose their editorial asides and a stray trailing comma in the data() call.

Tests

tests/testthat/test_lint.R runs again, wrapped in skip_on_cran(). It had been commented out entirely, so the suite enforced nothing about style locally even though CI kept its own lint job. The guard keeps it off the R CMD check clock.

Verification

• lintr::lint_package() — 0 lints
• NOT_CRAN=true VDIFFR_RUN_TESTS=true devtools::test() — 1468 pass, 0 fail, 5 skip
• vdiffr snapshots — 0 pruned
• R CMD check --as-cran with the manual, built from a clean git archive export — Status: 1 NOTE (the standard CRAN incoming feasibility note: maintainer name and update count; no package defect)
• Tarball 2.2 MB; check time ~3.8 min across timed steps, inside the sub-10-minute budget

Deliberately not in this PR

The ~35-line pbc munging block is still duplicated across four help pages. Deduplicating it means shipping a dataset or a helper — new package surface, and a minor-version decision rather than a patch one. It also costs example runtime on every check, so it is worth doing, just not here.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.02%. Comparing base (8348409) to head (a0171d9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
+ Coverage   88.94%   89.02%   +0.07%     
==========================================
  Files          50       50              
  Lines        4606     4618      +12     
==========================================
+ Hits         4097     4111      +14     
+ Misses        509      507       -2     

Files with missing lines	Coverage Δ
R/calc_roc.R	86.20% <100.00%> (+2.87%)	⬆️
R/gg_error.R	89.88% <ø> (ø)
R/gg_partial_rfsrc.R	90.51% <100.00%> (+0.28%)	⬆️
R/gg_roc.R	87.71% <ø> (ø)
R/gg_vimp.R	84.68% <ø> (ø)
R/plot.gg_error.R	100.00% <ø> (ø)
R/plot.gg_rfsrc.R	80.30% <ø> (ø)
R/plot.gg_roc.R	93.75% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR rolls several Codex review follow-ups into the 3.5.1 patch: it fixes two user-facing defects (gg_roc() with which_outcome = 0 on rfsrc, and gg_partial_rfsrc() erroring on invalid inputs), aligns ROC documentation across entry points, and restores a lint test guard in the test harness.

Changes:

• Fix: normalize which_outcome = 0 for the rfsrc ROC path so it behaves like "all" (warn + fallback to class 1) instead of silently producing a degenerate ROC object.
• Fix: add an upfront class check in gg_partial_rfsrc() to emit a consistent package error for non-rfsrc inputs.
• Docs/tests: expand README + NEWS, clarify ROC entry-point divergence in roxygen/Rd, and re-enable the lint test with skip_on_cran().

Reviewed changes

Copilot reviewed 13 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/testthat/test_lint.R	Re-enables style/lint test with CRAN skip guard.
tests/testthat/test_gg_roc.R	Adds regression coverage for which_outcome = 0 behavior on rfsrc.
tests/testthat/test_gg_partial_rfsrc.R	Adds regression coverage for early validation of invalid rf_model.
README.md	Expands and reorganizes function reference; adds 3.5.1 highlight.
R/plot.gg_roc.R	Updates documentation about ROC plotting behavior across entry points.
R/plot.gg_rfsrc.R	Cleans up pbc example commentary in roxygen.
R/plot.gg_error.R	Cleans up pbc example commentary and fixes data() call in roxygen.
R/gg_vimp.R	Cleans up pbc example commentary and fixes data() call in roxygen.
R/gg_roc.R	Clarifies which_outcome semantics and documents entry-point default differences.
R/gg_partial_rfsrc.R	Adds early class validation with a package-style error message.
R/gg_error.R	Cleans up pbc example commentary and fixes data() call in roxygen.
R/calc_roc.R	Normalizes which_outcome = 0 alongside "all" for the rfsrc ROC calculation path.
NEWS.md	Adds 3.5.1 changelog bullets covering defects, docs, and tests.
man/plot.gg_roc.Rd	Regenerated Rd reflecting updated plot ROC docs.
man/plot.gg_rfsrc.Rd	Regenerated Rd reflecting updated pbc example wording.
man/plot.gg_error.Rd	Regenerated Rd reflecting updated pbc example wording and data() call.
man/gg_vimp.Rd	Regenerated Rd reflecting updated pbc example wording and data() call.
man/gg_roc.rfsrc.Rd	Regenerated Rd reflecting updated ROC documentation and defaults section.
man/gg_error.Rd	Regenerated Rd reflecting updated pbc example wording and data() call.
man/calc_roc.rfsrc.Rd	Regenerated Rd reflecting updated which_outcome documentation.

Files not reviewed (7)

• man/calc_roc.rfsrc.Rd: Generated file
• man/gg_error.Rd: Generated file
• man/gg_roc.rfsrc.Rd: Generated file
• man/gg_vimp.Rd: Generated file
• man/plot.gg_error.Rd: Generated file
• man/plot.gg_rfsrc.Rd: Generated file
• man/plot.gg_roc.Rd: Generated file

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ehrlinger]

Confirmed and fixed in a0171d9. Verified the dispatch: randomForestSRC registers plot.rfsrc and randomForest registers plot.randomForest, so plot(rfsrc_fit) returns NULL rather than a ggplot — it never reaches this method.

The @param x text now states that plain plot(forest) dispatches to plot.rfsrc()/plot.randomForest(), and that this branch is reachable only by naming the method outright as plot.gg_roc(forest).

Worth noting this was not only my new wording — the pre-existing line "Hand it a forest and gg_roc() is called for you" had the same problem, so the correction goes back further than this PR.

[ehrlinger]

Fixed in a0171d9. The section now contrasts plot(gg_roc(x)) with a direct plot.gg_roc(x) call explicitly, and adds that plain plot(x) on a forest reaches neither, since it dispatches to the forest's own method in randomForestSRC or randomForest.

This also resolves an open question from the original review that prompted this PR. It asked whether the raw-forest path should be treated as public, on the grounds that the roxygen documented raw forests as valid x. Dispatch settles it: normal plot() usage never lands there, so the docs advertising it were what made it look public.

[ehrlinger]

Keeping this one as-is, on two grounds.

Runtime: I measured the fit rather than estimating it. rfsrc(Species ~ ., data = iris, ntree = 50) costs ~6 ms (5 fits in 0.031s elapsed). The duplicate is not a meaningful contributor to check time — the whole checking tests step is 37s, and the package's real budget pressure is the 64s vignette rebuild.

Idiom: per-test model construction is what this file already does. test_gg_roc.R grows the same iris forest independently at lines 108 and 245 as well, so the new test follows the local convention rather than departing from it. Introducing a file-level fixture would make these tests order-dependent for a saving in the millisecond range.

The two tests are also asserting different contracts — one guards that an explicit which_outcome is unchanged, the other that 0 and "all" converge — so merging them would couple a regression test to a characterization test.

Happy to revisit if a shared fixture lands for this file generally.