Repository navigation
fix: quote YAML 1.1 booleans, fix replaceMets, and align task lists, gene pruning and YAML floats with raven-toolbox - #11
Merged
Conversation
needsQuote now quotes y, n, yes, no, on and off in any case, which YAML 1.1 loaders such as PyYAML read as booleans. This matches the quoting of write_yaml_model in raven-toolbox (CompMetEng/raven-toolbox#178). #10
Contributor
Function test results432 tests 425 ✅ 1m 57s ⏱️ Results for commit 875e45b. ♻️ This comment has been updated with latest results. |
…es header case, keeps empty CHANGED bounds as NaN
… read back exactly
edkerk
changed the title
… that is left By name, replaceMets deletes the replacement's row and keeps the renamed replaced metabolite. The ids it passed to the final contractModel call (repMetIds) were captured from the replacement only, so after the deletion none of them was found, repIdx was empty and contractModel(model,'mets',[]) checked every reaction. Duplicate reactions that do not involve the merged metabolite were merged as well. repMetIds now also holds the id of the replaced metabolite, so the restriction follows whichever of the two rows survives, and an empty list no longer reaches contractModel. Tests: unrelated duplicate reactions survive a merge by name and by id, and the new duplicate is still contracted.
edkerk
changed the title
edkerk
added a commit
to CompMetEng/raven-gecko-parity
that referenced
this pull request
Oct 21, 2026
New scenarios: remove_low_score_genes_ties_smallyeast, task_list_edge_cases and yaml_float_digits_smallyeast. The task-list scenarios compare an empty CHANGED bound as NaN. Ledger notes for replaceMets, removeLowScoreGenes, parseTaskList, checkTasks, writeYAMLmodel, exportForGit and isDnfGrRule now say what CompMetEng/RAVEN#11 and CompMetEng/raven-toolbox#181 change.
edkerk
added a commit
to CompMetEng/raven-gecko-parity
that referenced
this pull request
Oct 21, 2026
…otes for the parity decisions (#139) * fix: name the solver of min_nr_fluxes_toy instead of requesting scip The MATLAB side called setRavenSolver('scip'), which RAVEN supports on Linux only if SCIP was compiled by hand, so the scenario ended in ERROR (no result from matlab) in every nightly. It now takes matlab_solver and python_solver from scenario.yml (gurobi, as in task_checking_smallyeast) and restores the previous RAVEN solver afterwards. * doc: record the differences confirmed on MATLAB in the ledger notes The first MATLAB batch on a cluster (RAVEN 79a7d8c, 2026-10-21) confirmed differences between RAVEN and raven-toolbox that the notes did not state or stated wrongly: replaceMets (a RAVEN bug, not replicated exactly), removeLowScoreGenes (the tie-break is alphabetical, not random), parseTaskList, checkTasks, writeYAMLmodel, exportForGit and isDnfGrRule. Only the notes of these seven rows change. * Add scenarios for the parity decisions and update the ledger notes New scenarios: remove_low_score_genes_ties_smallyeast, task_list_edge_cases and yaml_float_digits_smallyeast. The task-list scenarios compare an empty CHANGED bound as NaN. Ledger notes for replaceMets, removeLowScoreGenes, parseTaskList, checkTasks, writeYAMLmodel, exportForGit and isDnfGrRule now say what CompMetEng/RAVEN#11 and CompMetEng/raven-toolbox#181 change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10.
YAML booleans
src/io/writeYAMLmodel.mneedsQuote:NO,yes,onand the other YAML 1.1 booleans (y,n,yes,no,on,offin any case) were written unquoted, so a YAML 1.1 loader such as PyYAML read nitric oxide'sname: NOasFalse. They are quoted, astrueandfalsealready were. raven-toolbox quotes the same set since fix: four writer, task and gene-pruning problems from #178 raven-toolbox#179.[Yy][Ee][Ss]) instead of(?i), which in MATLAB would also make the numeric alternatives case-insensitive and quote values such as0X1Fthat raven-toolbox leaves plain.replaceMets bug
replaceMetscalledcontractModelon the whole model, so it also merged unrelated metabolites that happened to share a name or id. The call is now restricted to the metabolite that is left, and skipped when nothing is left to contract.tManipulation: by name and by id leave unrelated duplicates alone, and by name still contracts the new duplicate. Without the fixreplaceMetsByNameLeavesUnrelatedDuplicatesAlonefails; with it all pass.Parity decisions from the RAVEN vs raven-toolbox review
Each item has a test. The Python side is CompMetEng/raven-toolbox#181.
writeYAMLmodel: non-integers are written with the shortest digits that read back exactly (15 to 17 significant digits), so 1/3 is0.3333333333333333and not0.333333333333333. Python writes the same.removeLowScoreGenes: when all isozymes are negative and tied, the gene listed first in the rule is kept, not the alphabetically first one. Surviving genes keep rule order. The docstring no longer says the tie is random.parseTaskList:SHOULD FAILandPRINT FLUXread0,falseandno(any case) as false.;lists are trimmed and empty items dropped.CHANGED LBorCHANGED UBis NaN, meaning the bound is left unchanged.checkTasksandfitTasksskip NaN bounds. Before, an empty bound left the bound arrays shorter than the list of changed reactions.checkTasks: warns when a task metabolite matches several model metabolites (only the first is constrained; raven-toolbox constrains all and warns too).Checks
Run in MATLAB R2024b with GLPK on head
875e45b(cluster batch 2026-10-21, results in CompMetEng/raven-gecko-parity, branchhpc-results/2026-10-21, folderfollow-up/pr11/):tTasks7/7,tIO24/24,tManipulation85/85,tINIT31/31,tSyntax1/1, none failed or incomplete. The scenariosremove_low_score_genes_smallyeast,replace_metabolite_toy,task_checking_smallyeast,task_list_parsingandyaml_roundtrip_smallyeastMATCH against Python seeds 0, 1 and 2.Not checked: other MATLAB versions and solvers (Gurobi, SCIP).