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 exception when calling expand_obs() on Observable with no coeffs #418

Merged
merged 2 commits into from Feb 12, 2019

Conversation

Projects
None yet
3 participants
@FFroehlich
Copy link
Contributor

commented Feb 8, 2019

sympy.symbols(',') throws a ValueError

@coveralls

This comment has been minimized.

Copy link

commented Feb 8, 2019

Coverage Status

Coverage increased (+0.009%) to 79.076% when pulling 116a64f on FFroehlich:empty_observables into 2afc709 on pysb:master.

@alubbock
Copy link
Member

left a comment

A small unit test would be helpful to catch regressions. Here's a fairly minimal one that could go in test_core.py:

@with_model
def test_expand_obs_no_coeffs():
    Monomer('A')
    Observable('A_obs', A())
    # Test that expand_obs works with observable when no coefficients or species
    # are present
    A_obs.expand_obs()
@FFroehlich

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Added the unit test. One thing I am a bit worried about is that you can now call .expand_obs() on observables before pysb.bng.generate_equations is called (as done in the unit test) and it will always return 0. I think in that setting expand_obs() should actually throw an exception and not return 0. Yet, I don't see a good way of checking whether pysb.bng.generate_equations has been called as checking for empty species/reactions will yield false positives for empty models.

@alubbock

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

I agree the behaviour is not ideal, but model.reactions and model.species both currently work similarly (they return empty lists before network generation). Ideally we'd track model state and (re-)generate the network on demand, but that's a PR for another day.

@alubbock alubbock merged commit 8e67aea into pysb:master Feb 12, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 79.076%
Details

@FFroehlich FFroehlich deleted the FFroehlich:empty_observables branch Feb 12, 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.