From 9bcb2f4a521cf5ec2e866125f5b684549d273d46 Mon Sep 17 00:00:00 2001 From: Alex Lubbock Date: Wed, 24 Apr 2019 16:50:15 -0500 Subject: [PATCH] Fix invalid component handling (#431) Previously, if one added an invalid parameter, rule etc. that generated an exception in `__init__`, (e.g. a validation error), the component was still added to the model. In interactive mode, this is annoying, as the model has to be re-instantiated. Example: Model() Parameter('a', 'invalid_value') # raises ValueError assert len(model.parameters) == 0 # fixed in this PR Parameter('a', 1.0) # should succeed This PR fixes this situation by adding the Component to the Model (by calling `Component.__init__()`) last instead of first. --- pysb/core.py | 14 +++++++------- pysb/tests/test_core.py | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/pysb/core.py b/pysb/core.py index a3426b62a..db9898c01 100644 --- a/pysb/core.py +++ b/pysb/core.py @@ -263,8 +263,6 @@ class Monomer(Component): alphanumeric or underscores. """ def __init__(self, name, sites=None, site_states=None, _export=True): - Component.__init__(self, name, _export) - # Create default empty containers. if sites is None: sites = [] @@ -304,6 +302,7 @@ def __init__(self, name, sites=None, site_states=None, _export=True): self.sites = list(sites) self.site_states = site_states + Component.__init__(self, name, _export) def __call__(self, conditions=None, **kwargs): """ @@ -1100,8 +1099,8 @@ def __getnewargs__(self): return (self.name, self.value, False) def __init__(self, name, value=0.0, _export=True): - Component.__init__(self, name, _export) self.value = float(value) + Component.__init__(self, name, _export) def get_value(self): return self.value @@ -1157,7 +1156,6 @@ class Compartment(Component): """ def __init__(self, name, parent=None, dimension=3, size=None, _export=True): - Component.__init__(self, name, _export) if parent != None and isinstance(parent, Compartment) == False: raise Exception("parent must be a predefined Compartment or None") #FIXME: check for only ONE "None" parent? i.e. only one compartment can have a parent None? @@ -1166,6 +1164,7 @@ def __init__(self, name, parent=None, dimension=3, size=None, _export=True): self.parent = parent self.dimension = dimension self.size = size + Component.__init__(self, name, _export) def __repr__(self): return '%s(name=%s, parent=%s, dimension=%s, size=%s)' % ( @@ -1214,7 +1213,6 @@ class Rule(Component): def __init__(self, name, rule_expression, rate_forward, rate_reverse=None, delete_molecules=False, move_connected=False, _export=True): - Component.__init__(self, name, _export) if not isinstance(rule_expression, RuleExpression): raise Exception("rule_expression is not a RuleExpression object") validate_expr(rate_forward, "forward rate") @@ -1237,7 +1235,9 @@ def __init__(self, name, rule_expression, rate_forward, rate_reverse=None, for cp in rp.complex_patterns: if not cp.is_concrete(): raise ValueError('Product {} of synthesis rule {} is not ' - 'concrete'.format(cp, self.name)) + 'concrete'.format(cp, name)) + + Component.__init__(self, name, _export) def is_synth(self): """Return a bool indicating whether this is a synthesis rule.""" @@ -1391,11 +1391,11 @@ def __getnewargs__(self): return (self.name, self.expr, False) def __init__(self, name, expr, _export=True): - Component.__init__(self, name, _export) if not isinstance(expr, sympy.Expr): raise ValueError('An Expression can only be created from a ' 'sympy.Expr object') self.expr = expr + Component.__init__(self, name, _export) def expand_expr(self, expand_observables=False): """Return expr rewritten in terms of terminal symbols only.""" diff --git a/pysb/tests/test_core.py b/pysb/tests/test_core.py index 25976fc0b..ac8dfdc87 100644 --- a/pysb/tests/test_core.py +++ b/pysb/tests/test_core.py @@ -378,3 +378,44 @@ def test_rulepattern_match_none_against_state(): # so this should be a valid rule pattern A(phospho=None) + A(phospho=None) >> A(phospho=1) % A(phospho=1) + +@with_model +def test_invalid_rule(): + Monomer('A') + assert_raises(ExpressionError, Rule, 'r1', None >> A(), 1.0) + assert len(model.rules) == 0 + + Parameter('kf', 1.0) + assert_raises(Exception, Rule, 'r1', 'invalid_rule_expr', kf) + assert len(model.rules) == 0 + + +@with_model +def test_invalid_expression(): + assert_raises(ValueError, Expression, 'e1', 'invalid_expr') + assert len(model.expressions) == 0 + + +@with_model +def test_invalid_monomer_name(): + assert_raises(ValueError, Monomer, 'a', 123) + assert len(model.monomers) == 0 + + +@with_model +def test_invalid_parameter(): + assert_raises(ValueError, Parameter, 'a', 'invalid_value') + assert len(model.parameters) == 0 + + +@with_model +def test_invalid_compartment(): + assert_raises(Exception, Compartment, 'c1', 'invalid_parent') + assert len(model.compartments) == 0 + + +@with_model +def test_invalid_observable(): + assert_raises(InvalidReactionPatternException, + Observable, 'o1', 'invalid_pattern') + assert len(model.observables) == 0 \ No newline at end of file