fix: reportEnzymeUsage skips only fully-inactive enzymes in topAbsUsage - #460
Merged
Conversation
isscalar(find(carriedFlux)) was false both for zero flux-carrying reactions and for multiple ones, so an enzyme with no flux-carrying reaction at all took the same "combined usage" branch as a genuinely multi-reaction enzyme, padding in a placeholder row instead of being left out. Reconciled with geckopy's report_enzyme_usage: an enzyme is skipped only when every one of its reactions carries no flux. An enzyme with a mix of flux- and non-flux-carrying reactions is still reported, attributed to the flux-carrying subset -- already correct before this change, now covered by a regression test alongside the fix. testReportEnzymeUsageTopAbsUsageOutOfBounds_tc0019's all-zero-flux fixture now correctly returns zero rows instead of a padded count; updated its assertions and comment to match. Closes SysBioChalmers/raven-gecko-parity#18.
Unit test results46 tests 46 ✅ 7s ⏱️ Results for commit 2cecdea. ♻️ This comment has been updated with latest results. |
…port-enzyme-usage # Conflicts: # test/unit_tests/geckoCoreFunctionTests.m
This was referenced Aug 29, 2026
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.
reportEnzymeUsage'stopAbsUsagetable used to pad in a placeholder row for an enzyme with no flux-carrying reaction at all:isscalar(find(carriedFlux))is false both for zero matches and for multiple matches, so a fully-inactive enzyme took the same "combined usage" branch as a genuinely multi-reaction one.Reconciled with geckopy's
report_enzyme_usage: an enzyme is skipped only when every one of its reactions carries no flux. An enzyme with a mix of flux- and non-flux-carrying reactions is still reported, attributed to the flux-carrying subset — this part was already correct before this change; the new test covers it too, alongside the fix, so it can't silently regress.New test
testReportEnzymeUsageSkipsOnlyFullyInactiveEnzymes_tc0045builds a minimal ecModel-like fixture exercising all three cases directly (fully-inactive → skipped; one active reaction out of several → single-branch; two active out of three → combined-branch, split only across the active ones). Confirmed it (and the updatedtc0019) fail against the pre-fix code before restoring the fix.testReportEnzymeUsageTopAbsUsageOutOfBounds_tc0019's all-zero-flux fixture now correctly returns zero rows instead of a padded count on every call; updated its assertions and comment to match — its actual purpose (no crash on an out-of-rangetopAbsUsage) is unaffected.Addresses SysBioChalmers/raven-gecko-parity#18.