-
Notifications
You must be signed in to change notification settings - Fork 13
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
Update calibration methods, add tests. #31
Conversation
Can you elaborate? |
Sure. The calibration class that uses MBAR currently uses context as an argument in it's I may be able to move this so context isn't needed in |
Here is the test case that shows the issue: |
Ah, so you just need to fix up the API. Why not just replace |
For the sake of unifying the APIs, I think with a little bit of work, we can include an The one thing in which the classes fundamentally differ -- which will be the part that takes some work --is storing all the previous states. Right now It'll take some time, and for now it isn't high priority, since we can already use SAMS on-line. MBAR was just an interesting at-line alternative, and we could just run MBAR off-line if we wanted to. Not fixing this right now shouldn't hold up our development. |
Added reminder to #20. |
You've probably already done this, but have you compared the free energies you get with SAMS with the free energies you get with MBAR? This is an important validation test for the SAMS code. |
@gregoryross Very qualitatively yes, they seemed to match up, but I'd like to set up a test suite/example set that proves this. |
Added reminder for that to #20. |
MBAR calibration currently not working because it depends on context in the initialization. Will fix this at another time.