feat: check charge balance in getElementalBalance - #679
Merged
Conversation
RAVEN checked charge nowhere: getElementalBalance is elemental only and
parseFormulas strips +/- from formulas. getElementalBalance now also reports
chargeStatus (1 balanced, 0 unbalanced, -1 unknown) and chargeResidual, as
sibling fields rather than folded into balanceStatus, which removeBadRxns and
printModelStats read as an elemental verdict.
Both are masked to the metabolites that participate in each reaction. S is
sparse and 0*NaN is NaN, so an unmasked sum would let a single unset charge
anywhere in the model poison every reaction's residual (measured in R2024b).
Meanwhile RAVEN had two functions that rebalance charge, and both summed with
'omitnan':
model.S(Hc, rxnPos) = -sum(model.S(:, rxnPos) .* model.metCharges, 'omitnan');
addMets defaults metCharges to NaN, so 'omitnan' treated an unknown charge as
neutral and silently wrote the wrong coefficient onto the balancing
metabolite. Both now refuse, naming the metabolites whose charge is unset: an
unknown charge balance is not a zero one.
Function test results244 tests 222 ✅ 1m 4s ⏱️ Results for commit d6ab2f4. |
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.
Main improvements in this PR:
getElementalBalancenow also reportschargeStatus(1 balanced, 0 unbalanced, -1 unknown) andchargeResidual. RAVEN previously checked charge nowhere. They are sibling fields;balanceStatusis unchanged and stays elemental.scaleBiomassPseudoreactionandapplyConditionsummed charges with'omitnan', so a metabolite with an unset charge counted as neutral and the balancing metabolite silently got the wrong coefficient. Both now refuse and name the metabolites whose charge is unset.tQueries.mandtBiomass.m.Instructions on merging this PR:
develop3as target branch, and will be resolved with a squash-merge.