bhmm / legacy-bhmm-force-spectroscopy-manuscript

Bayesian hidden Markov models for analysis of single-molecule trajectory data
GNU Lesser General Public License v3.0
2 stars 3 forks source link

Api fixes and tests #17

Closed franknoe closed 9 years ago

franknoe commented 9 years ago

Large PR with a number of fixes and improvements. All tests pass on my machine, and the examples look fine. In order to generate output (both figures and tables) go to an example directory, e.g. `cd examples/p5ab-hairpin/', and do:

Major changes in the code:

New features:

Testing and docs:

Refactoring:

Examples for paper

Bugfixes

jchodera commented 9 years ago

This is an enormous PR, but looks very nice! Many thanks for putting this together.

In the future (when both of us have more availability for rapid code review!), let's try to make PRs more atomic and associated with single features. But this is totally fine for now.

I'd love to discuss where the plotting and data analysis tools should actually go. If they're meant to be general command-line scripts, we can actually move them into bhmm.scripts and deploy them on install into the user's path. Also, I do think it is very useful to have the general plotting tools installed, rather than keeping them in the examples directory---especially if they are to be used frequently. But we can discuss this in separate issues.

Merging!

franknoe commented 9 years ago

Am 05/05/15 um 03:22 schrieb John Chodera:

This is an enormous PR, but looks very nice! Many thanks for putting this together.

In the future (when both of us have more availability for rapid code review!), let's try to make PRs more atomic and associated with single features. But this is totally fine for now.

I totally agree. But this was actually just two days work - at times when you are available I can of course still subdivide it!

I'd love to discuss where the plotting and data analysis tools should actually go. If they're meant to be general command-line scripts, we can actually move them into |bhmm.scripts| and deploy them on install into the user's path. Also, I /do/ think it is very useful to have the general plotting tools installed, rather than keeping them in the examples directory---especially if they are to be used frequently. But we can discuss this in separate issues.

I agree but would like to avoid a hard dependency. We could have them as an optional dependency, check for their availability where we need them and raise an Error when they are not there. Alternatively, we could build a user package which depends on BHMM and everything else that may be useful and adds convenience functions, plotting functions and stuff for specific HMMs.

Best Frank

Merging!

— Reply to this email directly or view it on GitHub https://github.com/bhmm/bhmm/pull/17#issuecomment-98899182.


Prof. Dr. Frank Noe Head of Computational Molecular Biology group Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354 Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany