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

Add dashed edge when reaction rate is an expression with observables #458

Merged
merged 4 commits into from Oct 7, 2019

Conversation

@ortega2247
Copy link
Contributor

ortega2247 commented Aug 14, 2019

No description provided.

Copy link
Member

alubbock left a comment

Looks good. My only thought is whether the network might get overwhelming for models where an Observable matches a large number of species. Perhaps this feature should have an optional keyword argument, include_rate_species=False, and a --include-rate-species option for the command line?

@ortega2247

This comment has been minimized.

Copy link
Contributor Author

ortega2247 commented Sep 16, 2019

Added the keyword argument include_rate_species and the command line option as suggested. One think that I noticed is that for models in which a species is in the reaction reactants and in the reaction rate there would be only one edge between that species node and the reaction node. We could allow multiple edges between nodes, when include_rate_species=True, if we make the graph a multigraph. Thoughts??

bng_enzymatic.pdf
bng_enzymatic2.pdf

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 16, 2019

Codecov Report

Merging #458 into master will decrease coverage by 0.16%.
The diff coverage is 6.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
- Coverage   79.41%   79.25%   -0.17%     
==========================================
  Files          96       96              
  Lines        9616     9702      +86     
==========================================
+ Hits         7637     7689      +52     
- Misses       1979     2013      +34
Impacted Files Coverage Δ
pysb/tools/render_reactions.py 17.7% <6.89%> (-4.83%) ⬇️
pysb/pathfinder.py 48.31% <0%> (-7.53%) ⬇️
pysb/importers/sbml.py 79.48% <0%> (-0.26%) ⬇️
pysb/tests/test_bng.py 100% <0%> (ø) ⬆️
pysb/core.py 88.31% <0%> (+0.18%) ⬆️
pysb/bng.py 93.21% <0%> (+0.26%) ⬆️
pysb/tests/test_core.py 95.19% <0%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39fdaa4...2e6ff9f. Read the comment docs.

@alubbock

This comment has been minimized.

Copy link
Member

alubbock commented Sep 18, 2019

I think a multigraph is OK here. It's a more complete visualisation, and can always be simplified to a graph if needed.

The alternative would be to represent combined reaction/rate relationships another way, for example using a different type of edge (e.g. dash-dot).

Copy link
Member

alubbock left a comment

LGTM, thanks!

@alubbock alubbock merged commit 584b350 into pysb:master Oct 7, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alubbock alubbock deleted the LoLab-VU:network_expressions branch Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.