Heuristics fix by mduncans · Pull Request #97 · A2-ai/pharos

mduncans · 2026-02-27T19:28:12Z

This PR updates the RunHeuristic checks to better surface what happened in the run.

For example, Model is now parsing the $COV step to determine if the cov step is requested. If requested and no issues found in the lst file then covariance_step_aborted is Some(false). If cov step was never requested we now have None to indicate not relevant.

This plays out for hyperion summaries where we can now provide better information to the user based on Some(true/false) vs None.

ext.rs changes

added with_eigenvalue_number and with_fixed_flags methods to ExtReader
added has_eigenvalue_issues to determine if model has eigenvalue issues (any less than 0)

lst.rs chagnes

RunHeuristics now has default_for method to set better defaults based on the Model object extracted from lst file.
- e.g. if $COV requested default is now Some(false) instead of None
parse_run_heuristics has been removed and added as a method to RunHeuristics as apply_lst_signals
parse_lst has been removed as RunHeuristics needs both lst and ext file to give a correct RunHeuristics object.
RunHeuristics now has a from_run method to parse lst and grab the eigenvalue info from ext file.

This kinda breaks the naming/convention here with lst.rs, but I think going to ext file is better than attempting to grab the eigenvalues from the lst file, but open for changing.

lexer.rs changes

Covariance added to can_parse_content

model.rs changes

Model now has Covariance field

parser.rs changes

Covariance is added as a struct and parsed identically to Simulation. We could consolidate these, but we might use information from covariance step, for example grabbing which matrix R vs S for putting in a table.

…lse) cases.

Keats · 2026-03-02T08:47:53Z

LGTM to me, let's see what copilot says

Copilot

Pull request overview

Updates NONMEM run heuristics to distinguish “not applicable” (None) vs “ran and did/didn’t happen” (Some(true/false)), including parsing $COV from the model and deriving eigenvalue status from .ext where available.

Changes:

Add $COVARIANCE parsing support and persist it on Model as covariance: Option<Covariance>.
Refactor .lst parsing to compute heuristics using model-derived defaults + .ext eigenvalue inspection + .lst signal scanning.
Extend .ext parsing to detect eigenvalue issues using the eigenvalues and fixed-flag iterations, plus add test coverage and updated snapshots.

Reviewed changes

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

Show a summary per file

File	Description
components/nonmem/src/parsing/lexer.rs	Allow lexing content for `$COVARIANCE` records.
components/nonmem/src/parsing/model.rs	Add `Covariance` struct and `Model.covariance` field.
components/nonmem/src/parsing/parser.rs	Parse `$COVARIANCE` options into `Model.covariance`.
components/nonmem/src/output_files/ext.rs	Add eigenvalue detection helpers and tests; support reading eigenvalues/fixed-flags rows.
components/nonmem/src/output_files/lst.rs	Refactor heuristics computation; add `defaults_for`, `.ext` eigenvalue layer, and `.lst` signal application.
components/nonmem/src/output_files/mod.rs	Switch summary construction to `LstSummary::from_run(lst_path, ext_path)`.
components/nonmem/test_data/ext/eigenvalues/bad.ext	New `.ext` fixture containing a negative eigenvalue.
components/nonmem/test_data/ext/eigenvalues/good.ext	New `.ext` fixture containing only positive eigenvalues.
components/nonmem/test_data/ext/eigenvalues/no-eig.ext	New `.ext` fixture with no eigenvalue row present.
components/nonmem/src/parsing/snapshots/nonmem__parsing__lexer__tests__can_lex_mod_files@bql.mod.snap	Snapshot updated due to `$COV` content now being tokenized (not ignored).
components/nonmem/src/parsing/snapshots/nonmem__parsing__model__tests__can_parse_comments_type1.snap	Snapshot updated for new `Model.covariance` field.
components/nonmem/src/parsing/snapshots/nonmem__parsing__parser__tests__can_parse_mod_files@everything-prob.mod.snap	Snapshot updated for new `Model.covariance` field.
components/nonmem/src/parsing/snapshots/nonmem__parsing__parser__tests__can_parse_mod_files@everything.mod.snap	Snapshot updated for new `Model.covariance` field.
components/nonmem/src/parsing/snapshots/nonmem__parsing__parser__tests__can_parse_mod_files@example1.mod.snap	Snapshot updated to include parsed covariance options.
components/nonmem/src/parsing/snapshots/nonmem__parsing__parser__tests__can_parse_mod_files@multiline_table.mod.snap	Snapshot updated for new `Model.covariance` field.
components/nonmem/src/parsing/snapshots/nonmem__parsing__parser__tests__can_parse_mod_files@nmexample.mod.snap	Snapshot updated to include parsed covariance options.
components/nonmem/src/parsing/snapshots/nonmem__parsing__parser__tests__can_parse_mod_files@theta_extended.mod.snap	Snapshot updated for new `Model.covariance` field.
components/nonmem/src/output_files/snapshots/nonmem__output_files__lst__tests__can_parse_lst@CONTROL5.lst.snap	Snapshot updated for new heuristic defaulting behavior.
components/nonmem/src/output_files/snapshots/nonmem__output_files__lst__tests__can_parse_lst@acop.lst.snap	Snapshot updated for new heuristic defaulting behavior.
components/nonmem/src/output_files/snapshots/nonmem__output_files__lst__tests__can_parse_lst@bql.lst.snap	Snapshot updated for new heuristic defaulting behavior.
components/nonmem/src/output_files/snapshots/nonmem__output_files__lst__tests__can_parse_lst@saemimp.lst.snap	Snapshot updated for new heuristic defaulting behavior (incl. eigenvalue flags).
components/nonmem/src/output_files/snapshots/nonmem__output_files__tests__run002_baseline_no_comments@run002__run002.mod.snap	Snapshot updated for `RunHeuristics` now defaulting to `Some(false)` where applicable.
components/nonmem/src/output_files/snapshots/nonmem__output_files__tests__run002_hide_off_diagonals@run002__run002.mod.snap	Same as above.
components/nonmem/src/output_files/snapshots/nonmem__output_files__tests__run002_type1_comments@run002__run002.mod.snap	Same as above.
components/nonmem/src/output_files/snapshots/nonmem__output_files__tests__run002_type1_comments_hide_off_diags@run002__run002.mod.snap	Same as above.
components/nonmem/src/output_files/snapshots/nonmem__output_files__tests__run003_baseline_no_comments@run003__run003.mod.snap	Snapshot updated for `RunHeuristics` now defaulting to `Some(false)` where applicable.
components/nonmem/src/output_files/snapshots/nonmem__output_files__tests__run003_hide_off_diagonals@run003__run003.mod.snap	Same as above.
components/nonmem/src/output_files/snapshots/nonmem__output_files__tests__run003_type1_comments@run003__run003.mod.snap	Same as above.
components/nonmem/src/output_files/snapshots/nonmem__output_files__tests__run003_type1_comments_hide_off_diags@run003__run003.mod.snap	Same as above.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-02T08:58:40Z

components/nonmem/src/output_files/lst.rs

+            } else if line.contains("COVARIANCE STEP ABORTED")
+                || line.contains("Forcing positive definiteness")
+            {
+                self.covariance_step_aborted = Some(true);
+                self.eigenvalue_issues = Some(true);


apply_lst_signals treats the presence of "Forcing positive definiteness" as evidence that the covariance step was aborted by setting covariance_step_aborted = Some(true). In saemimp.lst this line appears without any "COVARIANCE STEP ABORTED" message, so this will report an abort even when the covariance step likely completed after forcing PD. Consider setting only eigenvalue_issues = Some(true) for this signal, and reserving covariance_step_aborted = Some(true) for explicit abort messages.

Suggested change

} else if line.contains("COVARIANCE STEP ABORTED")

|| line.contains("Forcing positive definiteness")

{

self.covariance_step_aborted = Some(true);

self.eigenvalue_issues = Some(true);

} else if line.contains("COVARIANCE STEP ABORTED") {

self.covariance_step_aborted = Some(true);

self.eigenvalue_issues = Some(true);

} else if line.contains("Forcing positive definiteness") {

self.eigenvalue_issues = Some(true);

This is a good question. I've reached out to scientists to get a more informed answer on what the expected behavior for this should be.

Copilot · 2026-03-02T08:58:40Z

components/nonmem/src/output_files/lst.rs

+        if let Ok(Some(has_issues)) = has_eigenvalue_issues(ext_path.as_ref()) {
+            run_heuristics.eigenvalue_issues = Some(has_issues);


LstSummary::from_run unconditionally attempts to parse the .ext file to compute eigenvalue issues. This can be expensive for large .ext files and is unnecessary when covariance (and therefore eigenvalue checks) is not applicable (e.g., when RunHeuristics::defaults_for left eigenvalue_issues as None). Consider gating this call on model.covariance.is_some() (or run_heuristics.eigenvalue_issues.is_some()) and/or ext_path.as_ref().exists().

Suggested change

if let Ok(Some(has_issues)) = has_eigenvalue_issues(ext_path.as_ref()) {

run_heuristics.eigenvalue_issues = Some(has_issues);

let ext_path = ext_path.as_ref();

if model.covariance.is_some() && ext_path.exists() {

if let Ok(Some(has_issues)) = has_eigenvalue_issues(ext_path) {

run_heuristics.eigenvalue_issues = Some(has_issues);

}

Copilot · 2026-03-02T08:58:40Z

components/nonmem/src/output_files/mod.rs

    let mut model = Model::parse(&fs::read_to_string(model_path)?)?;
    let parameter_names = model.get_parameter_names(comment_type)?;

-    let lst_summary = parse_lst(&fs::read_to_string(&lst_path)?);
+    let lst_summary = LstSummary::from_run(&lst_path, &ext_path)?;

    let shk_data = if shk_path.exists() {


get_summary now calls LstSummary::from_run(&lst_path, &ext_path), and later parses the same .ext file again via get_estimation_results. If .ext files are large, this doubles I/O and parse cost for every summary. Consider plumbing the eigenvalue information from the existing get_estimation_results parsing (or otherwise avoiding a second .ext parse) so summaries only read/parse the file once.

ah I didn't notice it was parsing the ext file again later!

mduncans added 7 commits February 27, 2026 10:49

parsing covariance step and improving RunHeuristics by adding Some(fa…

52d2819

…lse) cases.

added eigenvalue checks from ext file, rework LstSummary parsing

84948a3

add fixed_flags and restrict eigenvalues for num_fixed params

bbd0c79

downgrade to None if cov step was skipped?

61f40b8

fix typos and move lst line checks to method on RunHeuristics

ceda5a3

remove unused parse_lst_content

1470030

add test for eigenvalue issues

cd5484b

mduncans requested a review from Keats February 27, 2026 19:31

Keats requested a review from Copilot March 2, 2026 08:47

Copilot started reviewing on behalf of Keats March 2, 2026 08:48 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

move eigenvalue checks to lst file only

f0b8362

Keats approved these changes Mar 3, 2026

View reviewed changes

mduncans added 2 commits March 5, 2026 11:37

reorder of heuristic checks

a3362ea

update snapshot after heuristic fix

36530db

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heuristics fix#97

Heuristics fix#97
mduncans wants to merge 10 commits intomainfrom
heuristics-fix

mduncans commented Feb 27, 2026

Uh oh!

Keats commented Mar 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

mduncans Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

Keats Mar 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if let Ok(Some(has_issues)) = has_eigenvalue_issues(ext_path.as_ref()) {
		run_heuristics.eigenvalue_issues = Some(has_issues);

Conversation

mduncans commented Feb 27, 2026

ext.rs changes

lst.rs chagnes

lexer.rs changes

model.rs changes

parser.rs changes

Uh oh!

Keats commented Mar 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

mduncans Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Keats Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants