From d7144b74dad3f527a3519f30534b4f4feaa54dcb Mon Sep 17 00:00:00 2001 From: Liam Keegan Date: Tue, 14 Mar 2023 14:02:14 +0100 Subject: [PATCH 1/2] change operation from mul to minus to show incorrect results for this case --- src/sbml/test/TestSBMLTransforms.cpp | 6 +++--- src/sbml/test/test-data/multiple-functions.xml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sbml/test/TestSBMLTransforms.cpp b/src/sbml/test/TestSBMLTransforms.cpp index 276d276d31..50699da9ae 100644 --- a/src/sbml/test/TestSBMLTransforms.cpp +++ b/src/sbml/test/TestSBMLTransforms.cpp @@ -98,9 +98,9 @@ START_TEST (test_SBMLTransforms_replaceFD) safe_free(math); /* https://github.com/sbmlteam/libsbml/issues/299 */ - /* fd: f_relabelled(p, S1) = p * S1 */ + /* fd: f_relabelled(p, S1) = p - S1 */ /* ast: f_relabelled(S1, p) * compartmentOne / t */ - /* ast after replaceFD: p * S1 * compartmentOne / t */ + /* ast after replaceFD: (S1 - p) * compartmentOne / t */ // use initial assignment instead of reaction, because the reaction // got flagged as having invalid units @@ -114,7 +114,7 @@ START_TEST (test_SBMLTransforms_replaceFD) SBMLTransforms::replaceFD(&ast, fd); math = SBML_formulaToString(&ast); - fail_unless (!strcmp(math, "p * S1 * compartmentOne / t"), NULL); + fail_unless (!strcmp(math, "(S1 - p) * compartmentOne / t"), NULL); safe_free(math); /* one function definition - nested */ diff --git a/src/sbml/test/test-data/multiple-functions.xml b/src/sbml/test/test-data/multiple-functions.xml index 0e0019b61d..db1f13ca69 100644 --- a/src/sbml/test/test-data/multiple-functions.xml +++ b/src/sbml/test/test-data/multiple-functions.xml @@ -46,7 +46,7 @@ S1 - + p S1 From c74c3d715898d0677c6dc133ee6058cadf92f3e4 Mon Sep 17 00:00:00 2001 From: Liam Keegan Date: Tue, 14 Mar 2023 14:04:10 +0100 Subject: [PATCH 2/2] replace arguments at the same time instead of sequentially - add `ASTNode::replaceArguments` to replace all arguments at the same time - refactor shared functionality into `copyNode` - modify `SBMLTransforms::replaceBvars` to use this instead of replacing arguments sequentially --- src/sbml/SBMLTransforms.cpp | 59 ++------------ src/sbml/math/ASTNode.cpp | 151 ++++++++++++++++++------------------ src/sbml/math/ASTNode.h | 18 ++++- 3 files changed, 100 insertions(+), 128 deletions(-) diff --git a/src/sbml/SBMLTransforms.cpp b/src/sbml/SBMLTransforms.cpp index 7c40f85f0a..82f6726083 100644 --- a/src/sbml/SBMLTransforms.cpp +++ b/src/sbml/SBMLTransforms.cpp @@ -146,59 +146,14 @@ SBMLTransforms::replaceBvars(ASTNode * node, const FunctionDefinition *fd) noBvars = fd->getMath()->getNumBvars(); fdMath = *fd->getBody(); - // get names of all bvars - std::vector allBVars; - for (unsigned int i = 0; i < noBvars; ++i) - allBVars.push_back(fd->getArgument(i)->getName()); - - // get all names in the node - List* names = node->getListOfNodes((ASTNodePredicate)ASTNode_isName); - - // convert to std::vector - std::vector currentChildNames; - for (unsigned int j = 0 ; j < names->getSize(); ++j) - { - ASTNode* name = (ASTNode*)names->get(j); - currentChildNames.push_back(name->getName()); - } - delete names; - - // establish order in which to replace bvars, we want to ensure that - // no bvars are replaced before they are used - std::vector replaceOrder; - for (unsigned int i = 0; i < noBvars; ++i) - { - std::string currentArg = fd->getArgument(i)->getName(); - bool added = false; - for (unsigned int j = 0; j < currentChildNames.size(); ++j) - { - if (currentArg == currentChildNames[j]) - { - replaceOrder.insert(replaceOrder.begin(), i); - added = true; - break; - } - } - if (!added) - { - replaceOrder.push_back(i); - } - } - - unsigned int nodeCount = 0; - - // now replace in the order established above - std::vector::iterator it = replaceOrder.begin(); - for (; it != replaceOrder.end(); ++it) - { - unsigned int i = *it; - if (nodeCount < node->getNumChildren()) - { - fdMath.replaceArgument(fd->getArgument(i)->getName(), - node->getChild(nodeCount)); - } - ++nodeCount; + // get names of bvars and AST node to substitute for each + std::vector bVars; + std::vector astNodes; + for (unsigned int i = 0; i < noBvars; ++i){ + bVars.push_back(fd->getArgument(i)->getName()); + astNodes.push_back(node->getChild(i)); } + fdMath.replaceArguments(bVars, astNodes); (*node) = fdMath; } diff --git a/src/sbml/math/ASTNode.cpp b/src/sbml/math/ASTNode.cpp index ab76f813ca..b2c042f4aa 100644 --- a/src/sbml/math/ASTNode.cpp +++ b/src/sbml/math/ASTNode.cpp @@ -2759,100 +2759,103 @@ ASTNode::isWellFormedASTNode() const /** @endcond */ - -LIBSBML_EXTERN -void -ASTNode::replaceArgument(const std::string& bvar, ASTNode * arg) +static void copyNode(const ASTNode * source, ASTNode * dest) { - if (arg == NULL) - return; - else if (getNumChildren() == 0) - { - if (this->isName() && this->getName() == bvar) + if (source == NULL) { - if (arg->isName()) - { - this->setType(arg->getType()); - this->setName(arg->getName()); - } - else if (arg->isReal()) - { - this->setValue(arg->getReal()); - if (arg->isSetUnits()) + return; + } + if (source->isName()) + { + dest->setType(source->getType()); + dest->setName(source->getName()); + } + else if (source->isReal()) + { + dest->setValue(source->getReal()); + if (source->isSetUnits()) { - this->setUnits(arg->getUnits()); + dest->setUnits(source->getUnits()); } - } - else if (arg->isInteger()) - { - this->setValue(arg->getInteger()); - if (arg->isSetUnits()) + } + else if (source->isInteger()) + { + dest->setValue(source->getInteger()); + if (source->isSetUnits()) { - this->setUnits(arg->getUnits()); + dest->setUnits(source->getUnits()); } - } - else if (arg->isConstant()) - { - this->setType(arg->getType()); - } - else - { - this->setType(arg->getType()); - this->setName(arg->getName()); - for (unsigned int c = 0; c < arg->getNumChildren(); c++) + } + else if (source->isConstant()) + { + dest->setType(source->getType()); + } + else + { + dest->setType(source->getType()); + dest->setName(source->getName()); + for (unsigned int c = 0; c < source->getNumChildren(); c++) { - this->addChild(arg->getChild(c)->deepCopy()); + dest->addChild(source->getChild(c)->deepCopy()); } - } } +} + +LIBSBML_EXTERN +void +ASTNode::replaceArgument(const std::string& bvar, ASTNode * arg) +{ + if (getNumChildren() == 0 && this->isName() && this->getName() == bvar) + { + copyNode(arg, this); + return; } for (unsigned int i = 0; i < getNumChildren(); i++) { - if (getChild(i)->isName()) + if (getChild(i)->isName() && getChild(i)->getName() == bvar) { - if (getChild(i)->getName() == bvar) - { - if (arg->isName()) - { - getChild(i)->setType(arg->getType()); - getChild(i)->setName(arg->getName()); - } - else if (arg->isReal()) - { - getChild(i)->setValue(arg->getReal()); - if (arg->isSetUnits()) - { - getChild(i)->setUnits(arg->getUnits()); - } - } - else if (arg->isInteger()) + copyNode(arg, getChild(i)); + } + else + { + getChild(i)->replaceArgument(bvar, arg); + } + } +} + +LIBSBML_EXTERN +void +ASTNode::replaceArguments(const std::vector& bvars, std::vector& args) +{ + std::size_t n = bvars.size(); + if (getNumChildren() == 0) + { + for(std::size_t j=0; jsetValue(arg->getInteger()); - if (arg->isSetUnits()) - { - getChild(i)->setUnits(arg->getUnits()); - } + if (this->isName() && this->getName() == bvars[j]) + { + copyNode(args[j], this); + return; + } } - else if (arg->isConstant()) + } + for (unsigned int i = 0; i < getNumChildren(); i++) + { + bool child_replaced = false; + for(std::size_t j=0; jsetType(arg->getType()); + if (getChild(i)->isName() && getChild(i)->getName() == bvars[j]) + { + copyNode(args[j], getChild(i)); + child_replaced = true; + break; + } } - else + if (!child_replaced) { - getChild(i)->setType(arg->getType()); - getChild(i)->setName(arg->getName()); - for (unsigned int c = 0; c < arg->getNumChildren(); c++) - { - getChild(i)->addChild(arg->getChild(c)->deepCopy()); - } + getChild(i)->replaceArguments(bvars, args); } - } - } - else - { - getChild(i)->replaceArgument(bvar, arg); } - } } LIBSBML_EXTERN diff --git a/src/sbml/math/ASTNode.h b/src/sbml/math/ASTNode.h index e43a4bfa8c..3084304fc8 100644 --- a/src/sbml/math/ASTNode.h +++ b/src/sbml/math/ASTNode.h @@ -1796,7 +1796,7 @@ setValue(value, 0); * For example, if the formula in this ASTNode is x + y, * and the function is called with @c bvar = @c "x" and @c arg = an ASTNode * representing the real value @c 3. This method would substitute @c 3 for - * @c x within this ASTNode object, resulting in the forula 3 + y. + * @c x within this ASTNode object, resulting in the formula 3 + y. * * @param bvar a string representing the variable name to be substituted. * @param arg an ASTNode representing the name/value/formula to use as @@ -1805,8 +1805,22 @@ setValue(value, 0); LIBSBML_EXTERN void replaceArgument(const std::string& bvar, ASTNode * arg); + /** + * Replaces occurrences of each given name with the corresponding ASTNode. + * + * For example, if the formula in this ASTNode is x - y, + * and the function is called with bvars = {"x", "y"} and args = ASTNodes + * representing objects with names {"y", "x"}, the result would be y - x. + * + * @param bvars a vector of strings representing the variable names to be substituted. + * @param args a vector of ASTNodes representing the name/value/formula to use as + * a replacement for each variable name + */ + LIBSBML_EXTERN + void replaceArguments(const std::vector& bvars, std::vector& args); - /** @cond doxygenLibsbmlInternal */ + + /** @cond doxygenLibsbmlInternal */ /** * Sets the parent SBML object of this node. Is not recursive, and will not set the parent SBML object of any children of this node.