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

Fix BioNetGen importer and related tests #406

Merged
merged 6 commits into from Mar 21, 2019

Conversation

Projects
None yet
3 participants
@alubbock
Copy link
Member

commented Jan 10, 2019

  • Ensure PySB imported model trajectories match BNG ones (#401)
  • Employ reduced precision for checking models containing expressions
    with division. Sympy converts (x/y) to (x*y**(-1)), which results
    in lost precision where x and y are large.
  • Fix import for models which contain (dynamic) expressions dependent
    on constant expressions. Previously, the latter were converted
    to generic sympy symbols during import.
  • Respect MoveConnected keyword on BNGL import
  • Respect observables' species compartments on BNGL import
  • match_once flag shouldn't be set on observables which match species instead of molecules
  • Preserve parameters in expressions, rather than convert them to
    numeric values on import.

Fixes: #401

@coveralls

This comment has been minimized.

Copy link

commented Jan 10, 2019

Coverage Status

Coverage decreased (-1.1%) to 78.453% when pulling 297c468 on alubbock:fix_bng_import into b8e036c on pysb:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 10, 2019

Coverage Status

Coverage increased (+0.05%) to 79.06% when pulling 32c0ba9 on alubbock:fix_bng_import into 869e105 on pysb:master.

@jmuhlich
Copy link
Member

left a comment

Looks good, I just found a few minor cleanup opportunities in the touched code.

As an aside, I don't think the magnitude of x and y affects the difference between x/y and x*y**-1 by more than a few ulps, but I guess that compounds over a simulation trajectory when things align just right (or wrong). Or did you find specific cases where it does? If not, I'd prefer removing that part of the comment before the REDUCED_PRECISION initializer.

rtol=1e-8))
assert numpy.allclose(yfull1[species], yfull2[species], atol=1e-8,
rtol=1e-8)
print(numpy.allclose(yfull1[species], yfull2[species], atol=precision,

This comment has been minimized.

Copy link
@jmuhlich

jmuhlich Feb 19, 2019

Member

Do we need the print calls in here? Wouldn't logging be more appropriate? Nose can be set up to show log output on test failure.

@@ -317,7 +317,9 @@ def _parse_rules(self):
rule.rate_reverse = rev_rate

def _parse_expressions(self):
expr_namespace = {p.name: p.value for p in self.model.parameters}
expr_namespace = {p.name: p for p in self.model.parameters}

This comment has been minimized.

Copy link
@jmuhlich

jmuhlich Feb 19, 2019

Member

This can be drastically simplified since ComponentSet is dict-like and we aren't taking the .value of the parameters anymore:

expr_namespace = dict(
    self.model.parameters | self.model.expressions_constant()
    | self.model.observables
)

(Some of that simplification could have been done previously, too, I guess) Depending on what's using it downstream, you probably don't even need to turn the combined ComponentSet into a dict.

@alubbock

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

Thanks for the feedback, suggestions have been implemented.

alubbock added some commits Jan 10, 2019

Fix BioNetGen importer and related tests
* Ensure PySB imported model trajectories match BNG ones (#401)
* Employ reduced precision for checking models containing expressions
  with division. Sympy converts (x/y) to (x*y**(-1)), which results
  in lost precision where x and y are large.
* Fix import for models which contain (dynamic) expressions dependent
  on constant expressions. Previously, the latter were converted
  to generic sympy symbols during import.
* Preserve parameters in expressions, rather than convert them to
  numeric values on import.

Fixes: #401

@alubbock alubbock force-pushed the alubbock:fix_bng_import branch from 2ea1d4a to 9c2f281 Feb 19, 2019

@alubbock

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

It seems that the new fixed species support brought to light a couple of issues importing BNG's Motivating_example_cBNGL model: the MoveConnected wasn't read in, species compartments in Observables were ignored, and the match_once flag shouldn't be set on observables which match species instead of molecules.

All the above issues have now been fixed in this PR.

Show resolved Hide resolved pysb/tests/test_importers.py Outdated
Show resolved Hide resolved pysb/tests/test_importers.py Outdated
Show resolved Hide resolved pysb/tests/test_importers.py

@alubbock alubbock merged commit 3c68232 into pysb:master Mar 21, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alubbock alubbock deleted the alubbock:fix_bng_import branch Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.