Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve BioNetGen function expression parsing #456

Closed
wants to merge 1 commit into from

Conversation

jmuhlich
Copy link
Member

BNGL models (and also SBML due to the conversion path) with piecewise
expressions and logical/equality expressions will now parse into
sympy expressions correctly. Piecewise expressions ('if' in BNGL)
in particular would trigger an exception in the sympy parser.

@jmuhlich
Copy link
Member Author

Suggestions on a testing strategy for this code are welcome. Should we cook up a torture test SBML/BNGL file or just test the new function I added that now contains all the conversion logic?

BNGL models (and also SBML due to the conversion path) with piecewise
expressions and logical/equality expressions will now parse into
sympy expressions correctly. Piecewise expressions ('if' in BNGL)
in particular would trigger an exception in the sympy parser.

Fixes pysb#455
@alubbock
Copy link
Member

alubbock commented Sep 9, 2019

I'd suggest using a token conversion function rather than string replacement, wherever possible.

Here's my attempt, by modifying parse_bngl_expr and adding _convert_tokens:

def _convert_tokens(tokens, local_dict, global_dict):
    from tokenize import NAME, OP
    for pos, tok in enumerate(tokens):
        if tok == (NAME, 'if'):
            tokens[pos] = (NAME, '__bngl_if')
            local_dict['__bngl_if'] = sympy.Function('bngl_if')
        elif tok == (OP, '^'):
            tokens[pos] = (OP, '**')
        elif tok == (NAME, 'and'):
            tokens[pos] = (OP, '&')
        elif tok == (NAME, 'or'):
            tokens[pos] = (OP, '|')
    return tokens


def parse_bngl_expr(text, *args, **kwargs):
    """Convert a BNGL math expression string to a sympy Expr."""

    # Translate a few operators with simple text replacement.
    text = text.replace('()', '')
    text = text.replace('==', '=')
    # Use sympy to parse the text into an Expr.
    trans = (
        sympy_parser.standard_transformations
        + (sympy_parser.convert_equals_signs, _convert_tokens)
    )
    expr = sympy_parser.parse_expr(text, *args, transformations=trans, **kwargs)
    # Transforming 'if' to Piecewise requires subexpression rearrangement, so we
    # use sympy's replace functionality rather than attempt it using text
    # replacements above.
    expr = expr.replace(
        sympy.Function('bngl_if'),
        lambda cond, t, f: sympy.Piecewise((t, cond), (f, True))
    )
    # Check for unsupported constructs.
    if expr.has('time'):
        raise ValueError(
            "Expressions referencing simulation time are not supported"
        )
    return expr

@alubbock
Copy link
Member

alubbock commented Sep 9, 2019

For testing, here are a few unit tests as a start:

def test_parse_bngl_expression_if():
    x, y = sympy.symbols('x y')
    assert parse_bngl_expr('if(x>y, 1, 3)') == \
        sympy.Piecewise((1, x > y), (3, True))


def test_parse_bngl_expression_exponentiate():
    x, y = sympy.symbols('x y')
    assert parse_bngl_expr('x ^ y') == sympy.Pow(x, y)


def test_parse_bngl_expression_and_or_equals():
    x, y = sympy.symbols('x y')
    assert parse_bngl_expr('x and y') == sympy.And(x, y)
    assert parse_bngl_expr('x or y') == sympy.Or(x, y)
    assert parse_bngl_expr('x == y') == sympy.Eq(x, y)

@alubbock
Copy link
Member

alubbock commented Sep 9, 2019

There's also an issue where BioNetGen's parser is more weakly typed than sympy: Booleans get automatically converted to integers in BNG, but not in sympy, so an expression like (10 > 5) * 4 fails on import, but would get evaluated to 4 by BNG. I've attempted a fix for that in the _fix_booleans function in #412, since this issue crops up a lot with local functions.

@alubbock alubbock mentioned this pull request Sep 24, 2019
@alubbock
Copy link
Member

Included and extended in #412

@alubbock alubbock closed this Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants