biosimulators / Biosimulators_utils

Utilities for building standardized command-line interfaces for biosimulation software packages
https://docs.biosimulators.org/Biosimulators_utils
MIT License
4 stars 6 forks source link

infix math from libsedml uses '^' instead of '**' in python, validation fails #118

Closed jcschaff closed 1 year ago

jcschaff commented 2 years ago

if the MathML for a computeChange contains a 'pow', the infix notation from returned by libsedml.formulaToL3String() represents the pow operator as '^' and python represents it as '**'. During validation of a computed change, this infix representation (stored in change.math) is evaluated and fails if the operands are not compatible with bitwise XOR (e.g. if either operand is a float)

The suggested fix in Biosimulators_utils/sedml/math.py is to replace '^' with '**' as follows:

def compile_math(math):
    """ Compile a mathematical expression

    Args:
        math (:obj:`str`): mathematical expression

    Returns:
        :obj:`_ast.Expression`: compiled expression
    """
    if isinstance(math, str):
        math = (
            math
            .replace('&&', 'and')
            .replace('||', 'or')
            .replace('^', '**')
        )

    math_node = evalidate.evalidate(math,
                                    addnodes=VALID_MATH_EXPRESSION_NODES,
                                    funcs=MATHEMATICAL_FUNCTIONS.keys())
    compiled_math = compile(math_node, '<math>', 'eval')
    return compiled_math
luciansmith commented 2 years ago

There's other issues with the mathml-to-infix converter, most obviously the inclusion of units--you can get stuff like "1 mL * S1" and the like. And you'll also have issues with true, false, avogadro, other built-in functions... I'll make a list and file a new bug report.

jcschaff commented 2 years ago

@luciansmith it seems that we can avoid some of these issues if we work directly with the expression ASTNodes, or ask libsbml/libsedml to validate and/or evaluate the expression for us.

jcschaff commented 2 years ago

@jonrkarr please weigh in on this issue. This power operator is blocking VCell/Biosimulators interoperability.

jcschaff commented 1 year ago

I didn't read Lucian's post carefully enough - he suggested to file a new Issue for expanded expression compatibility support (e.g. units). I will merge the associated approved PR (by Eran) and close this issue.