DCPROGS / HJCFIT

Full maximum likelihood fitting of a mechanism directly to the entire sequence of open and shut times, with exact missed events correction.
GNU General Public License v3.0
7 stars 4 forks source link

Place time critical matrices on the stack. #128

Closed jenshnielsen closed 8 years ago

jenshnielsen commented 8 years ago

This significantly speeds up the single core performance of HJCFIT. For example I see an improvement of fitglyR4 from approx 380 seconds to 130 on my mac. Similarly the simple benchmark in https://github.com/jenshnielsen/HJCfit_benchmark goes from 17 sec to less than 6.

This means limiting the number of elements in the matrices to less than 50. As I understand it all these matrices are of the size n_shut, n_open or n_open + n_shut. In practice I don't think this is an issue but we can change the size if needed. To protect against out of memory access the constructors for log10likelihood, exact_survivor and asymptotes checks the supplied qmatrix size.

This naturally uses more memory but for example fitglyR4 still only uses 57 MB

jenshnielsen commented 8 years ago

This is not ready. Please do not merge yet

jenshnielsen commented 8 years ago

@raquel-ucl I think this is ready for review.

@mdavezac If you have time to look at this I would appreciate the feedback

jenshnielsen commented 8 years ago

The relevant part of the eigen documentation is in http://eigen.tuxfamily.org/dox/group__TutorialMatrixClass.html (Optional template parameters)

raquelalegre commented 8 years ago

I'm happy to merge. Should I wait for @mdavezac's feedback first?

jenshnielsen commented 8 years ago

Yes lets do that.

jenshnielsen commented 8 years ago

Benchmark on Archer. The upper blue line is the original develop branch. The lower is this branch.

download

mdavezac commented 8 years ago

Other than the name t_srmatrix, which I don't find particularly enlightening, this looks good to me. It's a really nice trick you guys found. It will come in useful in other projects.

jenshnielsen commented 8 years ago

I failed to come up with a good name any idea is welcome

mdavezac commented 8 years ago

t_stack_matrix too long?

mdavezac commented 8 years ago

It's not that important, though. So do merge.

jenshnielsen commented 8 years ago

I guess it should be t_stack_rmatrix (r for real I think) lets do that

jenshnielsen commented 8 years ago

retest this please