fix: restore a fit's ID column as a factor

[mattfidler]

Problem

The fit table round-trips through a plain .csv, so read.csv() brings ID back as an integer while a live fit carries a factor. Anything that joins the fit table against something derived from the fit then hits a type mismatch — nlme::augPred() keeps its id a factor.

ggPMX::pmx_nlmixr() on a cached fit died outright:

Error in `bmerge()`:
! Incompatible join types: x.ID (factor) and i.ID (integer).
  Factor columns must join to factor or character columns.

(nlmixr2's xgxr-nlmixr-ggpmx article, which loads its fits with :=.)

Fix

Put ID back to a factor in loadFit(). Doing it there rather than in the restore script written by saveFit() means caches written by earlier versions are repaired on load too — the restore script is a saved artifact inside the zip, so a save-side fix would only help fits saved from here on.

Levels come from ranef/etaObf, which the restore script has already put back as factors with the fit's own levels; failing that, from the order the IDs appear.

Tests

New assertions in test-save.R's round-trip block, for both the FOCEi and SAEM fits. Verified on the real cached article fit: ID restores as a factor with levels 31, 32, 33, ... taken from ranef, and the xgxr-nlmixr-ggpmx article now renders.

Pre-existing unrelated failure in that file (parHistData type factor levels — nlmixr2est has added "Analytic Gradient (relaxed)" and friends since the fallback level list was written) is unchanged by this PR; confirmed present on the base commit too.

🤖 Generated with Claude Code

[mattfidler]

Antigravity review (Gemini 3.1 Pro), 6 rounds to convergence

Ran agy over the branch diff repeatedly, fixing what survived triage each round, until two passes came back clean. Summary of what was found and what was done. The coverage CI failure is also fixed — it turned out to be a real bug in this package, not the unrelated pre-existing failure the PR description claimed.

Fixed

1. parHistData$type levels were never recorded for compressed fits (the coverage CI failure)

saveFit() read the levels straight out of the fit environment, but nlmixr2est stores parHistData compressed unless control=list(compress=FALSE) — so the env holds a raw vector, is.data.frame() was FALSE, and nothing was recorded. loadFit() then fell back to its hardcoded 9-level list, which nlmixr2est has outgrown ("Analytic Gradient (relaxed)" and friends), and those types came back as NA.

That is exactly the test-coverage failure on this branch and on main — it is not the unrelated failure the PR body describes. Fixed by decompressing with fit$parHistData first, the same way the save loop already handles every other raw item.

2. A cache whose own restore script dropped a type could not be repaired by re-saving

The type levels are applied by the restore script stored inside the zip, so an old cache coerces unknown types to NA on load — and re-saving cannot recover them, because by then the strings are gone. Repaired in loadFit() instead (the same reasoning that put the ID repair there): the -parHistData.csv still holds the original strings and has not been cleaned up yet, so the column is read back and the missing levels appended. Existing caches are fixed in place.

3. saveFit(fit, zip=FALSE) was silently ignored for any fit carrying data

saveFit.nlmixr2FitData() wrote the fit .csv and then called the core method with a hardcoded zip=TRUE. The documented argument did nothing for the common case.

4. Saving or loading one cache could delete another one's files (destructive)

The file list was matched with an unanchored pattern, so a cache named fit also matched myfit-env.R — saveFit() zipped the other cache's files into its own archive and then unlink()ed them. Anchoring alone was not enough: the base name is a variable name or a nlmixr2save.prefix, so it can hold regexp metacharacters — my.fit is an ordinary R name, and as a pattern its . also matches my_fit's files, while fit+1 matches nothing and leaves an unloadable cache. Now matched literally via startsWith(), in one helper instead of three copies.

This was reachable before, but fix 3 makes leftover unzipped files a normal occurrence, so it mattered more.

5. Smaller hardening

• The repair read the csv with inferred column types, so a level like "01" would come back as 1 and "T" as TRUE. Read as character.
• .nlmixr2saveRestoreIdFactor() applied the ranef levels directly; now unions them with the table's own IDs so a missing subject can never turn into NA while fixing the column's type.
• saveFit(.fit) zipped nothing — list.files() omits dot files, and .fit is an ordinary R name. (Predates this PR; the regexp had the same hole.)

Tests

• The round-trip test could not distinguish ranef-derived ID levels from the appearance-order fallback, because theo_sd's IDs appear in level order. Reversing the recorded levels makes the two disagree and pins down that ranef wins.
• The old-cache test asserted the restore script appends an unknown type, but loadFit()'s repair fixed it either way, so the test passed whether or not the script worked. It now sources the restore script directly to pin down the script's own behavior, then checks loadFit() separately.
• New: an old cache with neither ..id.level.. nor ..parHistType.level..; a cache whose own script drops a type; cross-cache file clobbering; and a fit-free unit test for the file matcher covering the metacharacter, dot-name, longer-name and nested-directory boundaries.

Reviewed and rejected

• read.csv() returning a factor when stringsAsFactors=TRUE — not reachable: read.table() has defaulted to stringsAsFactors=FALSE since R 4.0 regardless of the option, and the option itself is now deprecated.
• ranef/etaObf being raw in the ID repair — those are restored from read.csv, and the repair only ever runs on loaded fits, so they are never raw there.
• Degenerate base names (empty string, trailing slash) and case-insensitive-filesystem collisions requiring a hand-made file — not reachable in real use.

Full suite passes locally with NOT_CRAN=true.

🤖 Generated with Claude Code

[mattfidler]

Coverage CI: fixed, and it was two separate problems

test-coverage is green for the first time since 2026-07-19. It had two causes stacked on top of each other.

1. A real bug in this package (the failure the job was reporting)

nlmixr2est stores parHistData compressed unless control=list(compress=FALSE), so the fit environment holds a raw vector rather than a data frame. saveFit() read the type factor levels straight off that, found no data frame, and recorded nothing — leaving loadFit() to fall back to a hardcoded 9-level list that nlmixr2est has since outgrown. "Analytic Gradient (relaxed)" and friends came back as NA.

That is the test-save.R:464 failure, and it was failing on main too — not the unrelated pre-existing failure the PR description claimed. Fixed by decompressing before reading the levels, the way the save loop already handles every other raw item.

2. Tests I added in this PR were far too expensive on CI

After that fix the job started timing out at 20-25 minutes with no output. It did not reproduce locally: the same covr run was only 6-7% slower than the base commit at 11, 4 and 2 threads alike. A controlled experiment (re-running the base commit the same day: 3m23s) confirmed the cost was on this branch.

Timing markers on CI found it:

block	duration
all five nlmixr2 fits	81s
a cache saved before the levels...	28s
saving one fit leaves another cache...	2m23s
restored ID levels come from ranef	4m10s
a cache whose own script dropped...	>11m28s (killed)

The fits were never the problem. Each extra loadFit() rebuilds an rxode2 model — nearly free against a warm cache, which is why it was invisible locally, and costly against the cold cache CI starts from, which is why each test was slower than the last.

The repair functions are now exercised on hand-built objects instead: a synthetic environment for the parHistData repair, a synthetic fit table for the ID repair. Neither needs a fit, and both pin the behavior down harder than the round trips did — ranef's levels can be made to disagree with the row order (b,a vs a,b), which theo_sd's IDs never do. One end-to-end old-cache test is kept.

Result: 3m20s locally against the base commit's 3m41s, coverage up from 82.41% to 82.74%. On CI the job now runs in 6 minutes.

Also

• Removed 107 build artifacts I had accidentally committed here via git add -A — a covr/ tree carrying absolute paths from my machine, plus src/*.gcda coverage counters. The .gcda files are actively harmful: CI compiles with --coverage and merges gcov output on top of whatever it finds in the tree. Now in .gitignore and .Rbuildignore so it cannot recur.
• Added a 20-minute step timeout. Without it a hung test takes the runner down with it (exit 143, no output) and the always() steps never run, so there is nothing to diagnose from. It is what turned a silent 65-minute death into a step that fails and leaves its testthat output behind.

All checks green: R-CMD-check on all 5 platforms, pkgdown, test-coverage, codecov.

🤖 Generated with Claude Code