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

Refactored propensity calculations and caching reaction stoichiometries #29

Merged
merged 3 commits into from
Jan 11, 2019

Conversation

jmason42
Copy link
Contributor

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.

@jmason42
Copy link
Contributor Author

Forgot to mention - this is about a 3x speed-up on my machine.

@prismofeverything
Copy link
Member

@jmason42 Looks great, I'll test this out!

@prismofeverything
Copy link
Member

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
]
Copy link
Member

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?

Copy link
Contributor Author

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:

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.

Copy link
Member

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.

Copy link
Contributor Author

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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

@jmason42
Copy link
Contributor Author

jmason42 commented Jan 8, 2019

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.

@prismofeverything
Copy link
Member

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.

@jmason42
Copy link
Contributor Author

jmason42 commented Jan 8, 2019

I'd be fumbling badly through C/C++ so you'd need to take the lead on that.

@prismofeverything prismofeverything merged commit 0d525ac into master Jan 11, 2019
@1fish2 1fish2 deleted the misc branch December 10, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants