From b523118f3c529f574a18c88c6d5a62eefa875597 Mon Sep 17 00:00:00 2001 From: Sarah Keating Date: Wed, 24 Dec 2025 15:33:20 +0000 Subject: [PATCH 1/3] Prevents conversion with multi-compartment rate rules Adds a check to prevent rate rule conversion when species from different compartments appear in the same rate rule. This change ensures that the converter correctly handles models with multiple compartments and avoids generating invalid or unexpected results. Also, adds example models for testing. --- src/sbml/conversion/SBMLRateRuleConverter.cpp | 100 +++++++++++++++++- src/sbml/conversion/SBMLRateRuleConverter.h | 8 ++ .../test/TestSBMLRateRuleConverter.cpp | 18 +++- .../test-data/rr_rn_multi_compartments.xml | 25 +++++ .../rr_rn_multi_compartments_reactions.xml | 35 ++++++ 5 files changed, 180 insertions(+), 6 deletions(-) create mode 100644 src/sbml/conversion/test/test-data/rr_rn_multi_compartments.xml create mode 100644 src/sbml/conversion/test/test-data/rr_rn_multi_compartments_reactions.xml diff --git a/src/sbml/conversion/SBMLRateRuleConverter.cpp b/src/sbml/conversion/SBMLRateRuleConverter.cpp index c670dc4603..dfd4b72534 100644 --- a/src/sbml/conversion/SBMLRateRuleConverter.cpp +++ b/src/sbml/conversion/SBMLRateRuleConverter.cpp @@ -414,10 +414,15 @@ SBMLRateRuleConverter::isDocumentAppropriate(OperationReturnValues_t& returnValu if (mModel->getNumCompartments() > 1) { - mDocument->getErrorLog()->logError(ModelContainsMultipleCompartments, mDocument->getLevel(), - mDocument->getVersion(), "There are multiple compartments."); - returnValue = LIBSBML_OPERATION_FAILED; - return false; + if (speciesFromMultipleCompartmentsInSameRateRule()) + { + mDocument->getErrorLog()->logError(ModelContainsMultipleCompartments, mDocument->getLevel(), + mDocument->getVersion(), "There are multiple compartments with species in the same rate rule."); + returnValue = LIBSBML_OPERATION_FAILED; + return false; + } + returnValue = LIBSBML_OPERATION_SUCCESS; + return true; } // 3. the document is invalid @@ -434,6 +439,93 @@ SBMLRateRuleConverter::isDocumentAppropriate(OperationReturnValues_t& returnValu /** @cond doxygenIgnored */ +bool +SBMLRateRuleConverter::speciesFromMultipleCompartmentsInSameRateRule() +{ + listPairString compartmentSpeciesPairs = getCompartmentSpeciesPairs(); + listPairString VariablesRateRulePairs = getVariablesRateRulePairs(); + for (listPairStringIt it1 = compartmentSpeciesPairs.begin(); + it1 != compartmentSpeciesPairs.end(); ++it1) + { + for (listPairStringIt it2 = VariablesRateRulePairs.begin(); + it2 != VariablesRateRulePairs.end(); ++it2) + { + if (it1->second == it2->first) + { + // species as variable in rate rule found + for (listPairStringIt it3 = compartmentSpeciesPairs.begin(); it3 != compartmentSpeciesPairs.end(); ++it3) + { + if (it3->second == it2->first && it3 != it1) + { + // species from different compartment in same rate rule found + if (it3->first != it1->first) + { + return true; + } + } + } + } + } + } + return false; +} + +listPairString +SBMLRateRuleConverter::getCompartmentSpeciesPairs() +{ + listPairString compartmentSpeciesPairs; + for (unsigned int n = 0; n < mDocument->getModel()->getNumCompartments(); n++) + { + Compartment* comp = mDocument->getModel()->getCompartment(n); + std::string compId = comp->getId(); + for (unsigned int m = 0; m < mDocument->getModel()->getNumSpecies(); m++) + { + Species* spec = mDocument->getModel()->getSpecies(m); + if (spec->getCompartment() != compId) + { + continue; + } + pairString pair(comp->getId(), spec->getId()); + compartmentSpeciesPairs.push_back(pair); + } + } + + return compartmentSpeciesPairs; +} + +listPairString +SBMLRateRuleConverter::getVariablesRateRulePairs() +{ + listPairString VariablesRateRulePairs; + for (unsigned int n = 0; n < mDocument->getModel()->getNumRules(); n++) + { + Rule* rule = mDocument->getModel()->getRule(n); + if (rule->getType() != RULE_TYPE_RATE) + { + continue; + } + std::string varId = rule->getVariable(); + const ASTNode* math = rule->getMath(); + List* variables = math->getListOfNodes(ASTNode_isName); + for (ListIterator it = variables->begin(); it != variables->end(); ++it) + { + ASTNode* m = static_cast(*it); + std::string mId = m->getName(); + if (mId != varId) + { + continue; + } + else + { + pairString pair(varId, mId); + VariablesRateRulePairs.push_back(pair); + } + } + } + return VariablesRateRulePairs; +} + + void SBMLRateRuleConverter::addODEPair(std::string id, Model* model) { diff --git a/src/sbml/conversion/SBMLRateRuleConverter.h b/src/sbml/conversion/SBMLRateRuleConverter.h index 379d120d4c..01a142a1c7 100644 --- a/src/sbml/conversion/SBMLRateRuleConverter.h +++ b/src/sbml/conversion/SBMLRateRuleConverter.h @@ -83,6 +83,8 @@ typedef std::vector setCoeff; typedef std::vector > >::iterator setCoeffIt; typedef std::pair pairString; +typedef std::list< pairString > listPairString; +typedef std::list< pairString >::iterator listPairStringIt; typedef std::vector< std::vector > setRnCoeffs; @@ -227,6 +229,12 @@ class LIBSBML_EXTERN SBMLRateRuleConverter : public SBMLConverter bool useStoichiometryFromMath(); + // functions to deal with multiple compartments + bool speciesFromMultipleCompartmentsInSameRateRule(); + + listPairString getCompartmentSpeciesPairs(); + + listPairString getVariablesRateRulePairs(); // functions for populateODEinfo() diff --git a/src/sbml/conversion/test/TestSBMLRateRuleConverter.cpp b/src/sbml/conversion/test/TestSBMLRateRuleConverter.cpp index 5dc7d1a91a..83af920e8b 100644 --- a/src/sbml/conversion/test/TestSBMLRateRuleConverter.cpp +++ b/src/sbml/conversion/test/TestSBMLRateRuleConverter.cpp @@ -1135,10 +1135,24 @@ START_TEST(test_converter_errors_4) } END_TEST +START_TEST(test_rule_reaction_multi_compartment) +{ + std::string raterule_file(TestDataDirectory); + raterule_file += "rr_rn_multi_compartments.xml"; + + std::string reaction_file(TestDataDirectory); + reaction_file += "rr_rn_multi_compartments_reactions.xml"; + + bool result = test_rule_to_reaction(raterule_file, reaction_file); + + fail_unless(result == true); +} +END_TEST + Suite * create_suite_TestSBMLRateRuleConverter (void) { - bool testing = false; + bool testing = true; Suite *suite = suite_create("SBMLRateRuleConverter"); TCase *tcase = tcase_create("SBMLRateRuleConverter"); tcase_add_checked_fixture(tcase, RateRuleConverter_setup, @@ -1146,7 +1160,7 @@ Suite *suite = suite_create("SBMLRateRuleConverter"); if (testing) { - tcase_add_test(tcase, test_rule_reaction_07); + tcase_add_test(tcase, test_rule_reaction_multi_compartment); } else { diff --git a/src/sbml/conversion/test/test-data/rr_rn_multi_compartments.xml b/src/sbml/conversion/test/test-data/rr_rn_multi_compartments.xml new file mode 100644 index 0000000000..020adef151 --- /dev/null +++ b/src/sbml/conversion/test/test-data/rr_rn_multi_compartments.xml @@ -0,0 +1,25 @@ + + + + + + + + + + + + + + + 11 + + + + + 22 + + + + + diff --git a/src/sbml/conversion/test/test-data/rr_rn_multi_compartments_reactions.xml b/src/sbml/conversion/test/test-data/rr_rn_multi_compartments_reactions.xml new file mode 100644 index 0000000000..ccda1a3d79 --- /dev/null +++ b/src/sbml/conversion/test/test-data/rr_rn_multi_compartments_reactions.xml @@ -0,0 +1,35 @@ + + + + + + + + + + + + + + + + + + + 11 + + + + + + + + + + 22 + + + + + + From cc69bb35338a17e08056166ef0e1a057b197f76e Mon Sep 17 00:00:00 2001 From: Sarah Keating Date: Sun, 28 Dec 2025 11:44:25 +0000 Subject: [PATCH 2/3] multiple compartments now only fail Iif the rate rules refer to more than one --- src/sbml/conversion/SBMLRateRuleConverter.cpp | 47 ++++++++-------- .../test/TestSBMLRateRuleConverter.cpp | 53 +++++++++++++++++-- .../test/test-data/rn_rr_fail_90104.xml | 28 ++++------ .../test/test-data/rn_rr_fail_90104_1.xml | 25 +++++++++ .../test/test-data/rn_rr_pass_90104.xml | 34 ++++++++++++ 5 files changed, 143 insertions(+), 44 deletions(-) create mode 100644 src/sbml/conversion/test/test-data/rn_rr_fail_90104_1.xml create mode 100644 src/sbml/conversion/test/test-data/rn_rr_pass_90104.xml diff --git a/src/sbml/conversion/SBMLRateRuleConverter.cpp b/src/sbml/conversion/SBMLRateRuleConverter.cpp index dfd4b72534..2fd59f5ab4 100644 --- a/src/sbml/conversion/SBMLRateRuleConverter.cpp +++ b/src/sbml/conversion/SBMLRateRuleConverter.cpp @@ -443,23 +443,33 @@ bool SBMLRateRuleConverter::speciesFromMultipleCompartmentsInSameRateRule() { listPairString compartmentSpeciesPairs = getCompartmentSpeciesPairs(); - listPairString VariablesRateRulePairs = getVariablesRateRulePairs(); - for (listPairStringIt it1 = compartmentSpeciesPairs.begin(); - it1 != compartmentSpeciesPairs.end(); ++it1) + listPairString variablesRateRulePairs = getVariablesRateRulePairs(); + for (listPairStringIt it_c = compartmentSpeciesPairs.begin(); + it_c != compartmentSpeciesPairs.end(); ++it_c) { - for (listPairStringIt it2 = VariablesRateRulePairs.begin(); - it2 != VariablesRateRulePairs.end(); ++it2) + for (listPairStringIt it_r = variablesRateRulePairs.begin(); + it_r != variablesRateRulePairs.end(); ++it_r) { - if (it1->second == it2->first) + if (it_c->second == it_r->second) { - // species as variable in rate rule found - for (listPairStringIt it3 = compartmentSpeciesPairs.begin(); it3 != compartmentSpeciesPairs.end(); ++it3) + // species from compartment it_c->first is a participant in a rule it_r + std::string ruleVar = it_r->first; + std::string compartmentId = it_c->first; + + // check that the variable for this rule is in a same compartment + for (listPairStringIt it_c1 = compartmentSpeciesPairs.begin(); + it_c1 != compartmentSpeciesPairs.end(); ++it_c1) { - if (it3->second == it2->first && it3 != it1) + if (it_c1 == it_c) + { + // skip the pair we have already considered + continue; + } + if (it_c1->second == ruleVar) { - // species from different compartment in same rate rule found - if (it3->first != it1->first) + if (it_c1->first != compartmentId) { + // variable in different compartment return true; } } @@ -496,7 +506,7 @@ SBMLRateRuleConverter::getCompartmentSpeciesPairs() listPairString SBMLRateRuleConverter::getVariablesRateRulePairs() { - listPairString VariablesRateRulePairs; + listPairString variablesRateRulePairs; for (unsigned int n = 0; n < mDocument->getModel()->getNumRules(); n++) { Rule* rule = mDocument->getModel()->getRule(n); @@ -511,18 +521,11 @@ SBMLRateRuleConverter::getVariablesRateRulePairs() { ASTNode* m = static_cast(*it); std::string mId = m->getName(); - if (mId != varId) - { - continue; - } - else - { - pairString pair(varId, mId); - VariablesRateRulePairs.push_back(pair); - } + pairString pair(varId, mId); + variablesRateRulePairs.push_back(pair); } } - return VariablesRateRulePairs; + return variablesRateRulePairs; } diff --git a/src/sbml/conversion/test/TestSBMLRateRuleConverter.cpp b/src/sbml/conversion/test/TestSBMLRateRuleConverter.cpp index 83af920e8b..bf66c3364b 100644 --- a/src/sbml/conversion/test/TestSBMLRateRuleConverter.cpp +++ b/src/sbml/conversion/test/TestSBMLRateRuleConverter.cpp @@ -1113,7 +1113,7 @@ START_TEST(test_converter_errors_3) delete d; } END_TEST -START_TEST(test_converter_errors_4) + START_TEST(test_converter_errors_4) { std::string raterule_file(TestDataDirectory); raterule_file += "rn_rr_fail_90104.xml"; @@ -1134,6 +1134,48 @@ START_TEST(test_converter_errors_4) delete d; } END_TEST +START_TEST(test_converter_errors_5) +{ + std::string raterule_file(TestDataDirectory); + raterule_file += "rn_rr_pass_90104.xml"; + + SBMLDocument* d = readSBMLFromFile(raterule_file.c_str()); + + rule_rn_props.addOption("useStoichiometryFromMath", true); + rule_rn_converter->setProperties(&rule_rn_props); + + + rule_rn_converter->setDocument(d); + int ret = rule_rn_converter->convert(); + fail_unless(ret == LIBSBML_OPERATION_SUCCESS); + fail_unless(d->getNumErrors() == 0); + + delete d; +} +END_TEST + +START_TEST(test_converter_errors_6) +{ + std::string raterule_file(TestDataDirectory); + raterule_file += "rn_rr_fail_90104_1.xml"; + + SBMLDocument* d = readSBMLFromFile(raterule_file.c_str()); + + rule_rn_props.addOption("useStoichiometryFromMath", true); + rule_rn_converter->setProperties(&rule_rn_props); + + + rule_rn_converter->setDocument(d); + int ret = rule_rn_converter->convert(); + fail_unless(ret == LIBSBML_OPERATION_FAILED); + fail_unless(d->getNumErrors() == 1); + fail_unless(d->getError(0)->getErrorId() == 90104); + fail_unless(d->getError(0)->getCategory() == LIBSBML_CAT_RATE_RULE_CONVERSION); + + delete d; +} +END_TEST + START_TEST(test_rule_reaction_multi_compartment) { @@ -1152,15 +1194,15 @@ END_TEST Suite * create_suite_TestSBMLRateRuleConverter (void) { - bool testing = true; + bool testing = false; Suite *suite = suite_create("SBMLRateRuleConverter"); TCase *tcase = tcase_create("SBMLRateRuleConverter"); tcase_add_checked_fixture(tcase, RateRuleConverter_setup, RateRuleConverter_teardown); - + if (testing) { - tcase_add_test(tcase, test_rule_reaction_multi_compartment); + tcase_add_test(tcase, test_converter_errors_6); } else { @@ -1198,6 +1240,9 @@ Suite *suite = suite_create("SBMLRateRuleConverter"); tcase_add_test(tcase, test_converter_errors_2); tcase_add_test(tcase, test_converter_errors_3); tcase_add_test(tcase, test_converter_errors_4); + tcase_add_test(tcase, test_converter_errors_5); + tcase_add_test(tcase, test_converter_errors_6); + tcase_add_test(tcase, test_rule_reaction_multi_compartment); diff --git a/src/sbml/conversion/test/test-data/rn_rr_fail_90104.xml b/src/sbml/conversion/test/test-data/rn_rr_fail_90104.xml index 41d03fe7e5..c8f83656e5 100644 --- a/src/sbml/conversion/test/test-data/rn_rr_fail_90104.xml +++ b/src/sbml/conversion/test/test-data/rn_rr_fail_90104.xml @@ -6,29 +6,21 @@ - + + + - - - - - - + - - - 2 - t - + y_3 - - - time + + + 22 - - - + + diff --git a/src/sbml/conversion/test/test-data/rn_rr_fail_90104_1.xml b/src/sbml/conversion/test/test-data/rn_rr_fail_90104_1.xml new file mode 100644 index 0000000000..0446be25aa --- /dev/null +++ b/src/sbml/conversion/test/test-data/rn_rr_fail_90104_1.xml @@ -0,0 +1,25 @@ + + + + + + + + + + + + + + + + + + y_2 + y_3 + + + + + + diff --git a/src/sbml/conversion/test/test-data/rn_rr_pass_90104.xml b/src/sbml/conversion/test/test-data/rn_rr_pass_90104.xml new file mode 100644 index 0000000000..41d03fe7e5 --- /dev/null +++ b/src/sbml/conversion/test/test-data/rn_rr_pass_90104.xml @@ -0,0 +1,34 @@ + + + + + + + + + + + + + + + + + + + + + 2 + t + + + + + + time + + + + + + From 141be53b56e44cfbe244ed99e5f7ad41a9105272 Mon Sep 17 00:00:00 2001 From: Sarah Keating Date: Sun, 28 Dec 2025 12:14:11 +0000 Subject: [PATCH 3/3] Refactors listPairString to use std::vector particularly useful given that std::list it does not exist in older versions --- src/sbml/conversion/SBMLRateRuleConverter.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sbml/conversion/SBMLRateRuleConverter.h b/src/sbml/conversion/SBMLRateRuleConverter.h index 01a142a1c7..4ff5a3b807 100644 --- a/src/sbml/conversion/SBMLRateRuleConverter.h +++ b/src/sbml/conversion/SBMLRateRuleConverter.h @@ -83,8 +83,8 @@ typedef std::vector setCoeff; typedef std::vector > >::iterator setCoeffIt; typedef std::pair pairString; -typedef std::list< pairString > listPairString; -typedef std::list< pairString >::iterator listPairStringIt; +typedef std::vector< pairString > listPairString; +typedef std::vector< pairString >::iterator listPairStringIt; typedef std::vector< std::vector > setRnCoeffs;