Skip to content

Add ferx as NLME fitting backend - #58

Merged
JordanBrooks33 merged 3 commits into
mainfrom
add-ferx
Jul 1, 2026
Merged

Add ferx as NLME fitting backend#58
JordanBrooks33 merged 3 commits into
mainfrom
add-ferx

Conversation

@JordanBrooks33

@JordanBrooks33 JordanBrooks33 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Integrate ferx (Rust-based NLME engine) as a new model-fitting backend alongside pharmpy, PsN, and nlmixr2
  • New functions: luna_run_ferx() (run models, save {id}-fit.rds), luna_ferx_info() (print convergence/OFV summary)
  • ferx paths added to luna_check(), get_status(), get_all_results(), update_cache(), runs_as_table()
  • Example project (example-4-ferx) with 3 busulfan models (1-cmt, 2-cmt, 2-cmt+WT allometry) and workflow vignette
  • 22 unit tests covering all ferx code paths using mockery stubs (no ferx package needed to run tests)
  • Updated ferx DSL syntax (one_cpt_infusionone_cpt_iv, two_cpt_infusiontwo_cpt_iv)

Configuration

Set in project YAML:

project:
  config:
    tools:
      modelfit:
        method: ferx
        data: path/to/data.csv

Test plan

  • devtools::test(filter = "ferx") — all 22 tests pass
  • Full test suite passes
  • CI checks pass (pkgdown + R CMD check)
  • Manual walkthrough of vignettes/example-4-ferx/luna_workflow_ferx.R with ferx installed

🤖 Generated with Claude Code

Integrate ferx (FeRx-NLME/ferx-r) as a new model-fitting backend
alongside pharmpy, PsN, and nlmixr2.

New functions:
- luna_run_ferx(): run ferx models, save results as {id}-fit.rds
- luna_ferx_info(): print convergence/OFV summary from saved results

ferx paths added to: luna_check(), get_status(), get_all_results(),
update_cache(), runs_as_table().

Includes example project (example-4-ferx) with 3 busulfan models
and a full workflow vignette, plus 22 unit tests using mockery stubs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread R/luna_run.R Outdated
c(list(model = model_file, data = data_path), dots))

result_file <- file.path(folder, paste0(id, "-fit.rds"))
saveRDS(result, result_file)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would consider not saving fit results as .rds, but using ferx-builtin saving feature. FeRx itself has an output= argument that allows saving to a cross-environment file format with .fitrx extension (essentially a zip file with yaml and json in it). This would be more universal and forward-looking (perhaps someone wants to use luna from command line).

@roninsightrx

Copy link
Copy Markdown
Collaborator

Code review — add-ferx (high effort, workflow-backed)

8 finder angles → independent verify pass. 10 findings kept, 9 refuted. Ranked most-severe first.

🔴 Release blockers (crash on existing functionality)

1. R/luna_run.R:70 — undefined get_nm_data_path()
data_path <- get_nm_data_path(model_file) runs unconditionally before run_nlme() on every non-ferx path. Function is defined nowhere in luna, not in NAMESPACE, not exported by pharmr.extra; result is never even used. Any NONMEM fit (including default method = "pharmpy") aborts: could not find function "get_nm_data_path". The package's primary existing feature is fully broken.

2. R/luna_check.R:81run_nlme() missing required id
Rewritten call passes only (model, check_only, verbose), dropping id/path/method. In pharmr.extra::run_nlme, id has no default and is forced early via set_name(model, new_name = id) and prepare_run_folder(id = id), both before the check_only branch. Every NONMEM check crashes: argument "id" is missing, with no default. Also path now defaults to getwd() → NM-TRAN check artifacts scatter into the working directory.

3. R/runs_as_table.R:74select(model_file, ofv) on a missing column
When the project YAML lists runs but no model files exist yet (fresh project, or runs added before .ferx/.mod files), get_all_results() returns an empty 0-column tibble. The new guard adds an ofv column but not model_file, so dplyr::select(model_file, ofv) aborts: Can't subset columns that don't exist: model_file. Crashes luna_runs() instead of showing an empty/pending table.

🟠 Correctness

4. R/get_status.R:20 — ferx "finished" ignores convergence
Status is "finished" whenever a -fit.rds exists, regardless of convergence or error. A non-converged or mid-fit-errored ferx run is reported as success → users interpret garbage parameter estimates.

5. R/luna_check.R:88 — lost NM-TRAN diagnostic text
Removed cat(attr(model_ok, "message")). On a syntax error luna now prints only the generic Model failed NONMEM compilation check.. The detailed NM-TRAN message (which line/record is wrong) is gone, so users can't diagnose the failure.

6. R/get_luna_config.R:15 — project config override now active
Switched .luna_cache$get("yaml") (a key never set → always NULL → project overrides were silent no-ops) to .luna_cache$get("project")$yaml. Project-level config blocks now actually take effect → existing projects may suddenly dispatch a different backend or change console behavior vs prior releases. Confirm this is intended.

🟡 Plausible / behavior change

7. R/luna_check.R:69 — soft fail replaces abort
Non-pharmpy model / missing dataset changed from cli::cli_abort() to invisible(FALSE) + warning. luna is the low-level layer consumed by pharmaair / the agent pipeline; a caller that doesn't inspect the return value now silently proceeds past a bad model.

8. R/luna_run.R:156 — brittle ferx method regex
luna_run_ferx re-parses method from the .ferx [fit_options] section with hand-rolled grep/sub. ferx::ferx_fit already reads that file → duplicate parsing at the wrong layer. The regex mis-parses quoted values, a method token in another section, or multi-line options → wrong/empty method=, fitting with the wrong estimation algorithm, no warning.

🟡 Cleanup

9. DESCRIPTION — version not bumped
Still Version: 0.1.0 despite substantial changes (and the DESCRIPTION diff itself edits Suggests/Remotes). Violates the CLAUDE.md rule "Update the package version in DESCRIPTION for every code change." Downstream pins / Docker caches keyed on version reuse the old build → ferx backend silently absent.

10. R/luna_run.R:117 — unused force param
luna_run_ferx(id, folder, config, force = FALSE, ...) never reads force and never forwards it to ferx::ferx_fit. force = TRUE is a silent no-op (and missing from the generated .Rd). Drop it or wire it through.


Refuted as false positives: runs_as_table.R:153 (ferx ofv numeric(0)), luna_run.R:178 (-fit.rds filename duplication), plus 7 other DRY/cleanup candidates. Findings 1–3 should block merge.

🤖 Generated with Claude Code

@roninsightrx

Copy link
Copy Markdown
Collaborator

The patch breaks existing NONMEM execution paths: luna_run() hits an undefined helper, and luna_check() calls run_nlme() without required context. These runtime failures affect default/non-ferx workflows.

Review findings:

  • [P1] Remove undefined data path lookupR/luna_run.R:70
    For any non-ferx luna_run() path (pharmpy, psn, or nmfe), execution now reaches get_nm_data_path(model_file) before calling run_nlme(). That helper is not defined anywhere in this package or nearby dependencies, and the resulting data_path is never used. Existing NONMEM runs now fail with could not find function "get_nm_data_path" after the model loads.

  • [P2] Pass run context into NONMEM checkR/luna_check.R:81
    When luna_check() is used with a NONMEM method, this call no longer supplies the required id or project path to pharmr.extra::run_nlme(). run_nlme() has a required id argument and uses it while preparing the check-only run folder, so valid luna_check("run1") calls now fail instead of performing the NM-TRAN syntax check.

Switch ferx result storage from saveRDS/{id}-fit.rds to ferx's native
.fitrx format via ferx_fit(output=) and ferx_load_fit(). Revert
luna_check() NONMEM validation from soft-fail (cli_alert_warning +
return FALSE) back to cli_abort() per PR review.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Kept ferx dispatch in luna_check and luna_run while incorporating
main's code review improvements (tryCatch, cli_alert_warning, nmfe
path resolution).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@roninsightrx roninsightrx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@JordanBrooks33
JordanBrooks33 merged commit 44db4ff into main Jul 1, 2026
2 checks passed
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.

2 participants