-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactored propensity calculations and caching reaction stoichiometries #29
Conversation
…ing the expensive np.where calls
Forgot to mention - this is about a 3x speed-up on my machine. |
@jmason42 Looks great, I'll test this out! |
Hey @jmason42, thanks for this. It did result in a significant speedup (one generation went from 22 minutes to 18). This is still far longer than the current simulation time of 8.5 minutes.... I know that we are okay adding time to the simulation if it results in a better system overall, but I feel this slowdown is maybe still outside the realm of reason, especially if we want to expand the algorithm to other processes. I'll try the sparse approach next. BTW, any thoughts on an approach like this? https://www.ncbi.nlm.nih.gov/pubmed/19162916 FPGA implementation of Gillespie with 100 million time steps per second? Seems like something that would be valuable to us. Who are the FPGA people at Stanford? |
reactants = [ | ||
np.where(stoichiometry < 0)[0] | ||
for stoichiometry in stoichiometric_matrix.T | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is great but I just can't get behind these dangling braces. PEP8 itself recommends keeping them on the same line as the one before. Do you have any other reason for preferring the dangling brace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, several reasons. Symmetry, for one. If the opening brace has no following characters on that line, then, likewise, I expect to see no leading characters on the line with the closing brace. It's also easier to add new lines - e.g. in a list of items, I'm not constantly pushing the brace to a new line, it's already where it needs to be. Further I find it easier to read and identify levels of precedence if the brace is given a new line. Finally, if I have reason to comment out individual items (maybe each line is an element in a list, or an optional argument to a function), it's much easier to disable the last item if I don't then have to migrate a bracket. I actually encountered specifically this issue during development - I wanted to run the test systems but only the first one until I finished debugging, but with the brace on the same line as the last element, toggling wasn't so easy:
arrow/arrow/test/test_arrow.py
Lines 120 to 124 in 654a269
systems = ( | |
test_equilibration, | |
test_dimerization, | |
test_complexation | |
) |
I could be missing it but I can't find an explicit recommendation one way or another in PEP 8. In fact all I can find is the explicit optional choice of putting a trailing brace on either the same level of precedence as the items, or at the level below. I've always had it at the same level simply because that's what Sublime Text does by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm looking back through it I was basing this on all of the code examples they give where they keep the paren on the same line:
# Aligned with opening delimiter.
foo = long_function_name(var_one, var_two,
var_three, var_four)
# More indentation included to distinguish this from the rest.
def long_function_name(
var_one, var_two, var_three,
var_four):
print(var_one)
# Hanging indents should add a level.
foo = long_function_name(
var_one, var_two,
var_three, var_four)
But it looks like they make a distinction between parens and braces? Which seems silly. Sigh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether they are making a distinction, or being vague.
|
||
return combinations | ||
|
||
def multichoose(n, k): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
Yeah, this doesn't really solve the performance issues to our desired level. As far as the FPGA goes, I'm not sure we need that level of performance - I think getting this properly compiled will be enough. That said I don't see Numba working going forward (although I don't really understand where the performance hangups are), and Cython has its own frustrations. |
Yeah I got the cython working but that makes it hard to distribute (pypi rejected my build due to compiled artifacts.... there is a way around it but moderately cumbersome). Could be worth it if it gives us the needed speedup? But currently it isn't. The other option is just to implement directly in C without numpy or anything (probably using the sparse matrix approach). I went through a few of the other existing native libraries (like Stochsim: https://lenoverelab.org/perso/lenov/stochsim.html) but the interface is so bad (can only specify systems through SBML???) that I think it could be worth implementing exactly what we need with the interface we want. Might break out the old c++ compiler just to see what the performance difference would be. |
I'd be fumbling badly through C/C++ so you'd need to take the lead on that. |
It was bugging me that propensities were called 'distributions' and combinations were called 'propensities', so I reorganized that logic (and moved all the pure math stuff to a
math
submodule). Once this was done, it was easy to start caching the reaction/reactant relationships. This work might collide with any attempt to make the representation more sparse, but it might also do a good chunk of the work.I also flattened some logic and replaced some defaults with
None
.