search

choderalab / openmmtools

A batteries-included toolkit for the GPU-accelerated OpenMM molecular simulation engine.
http://openmmtools.readthedocs.io
MIT License
253 stars 81 forks source link

add entropy/enthalpy support for pymbar4 #758

Open schuhmc opened 4 weeks ago

schuhmc commented 4 weeks ago

Description

Fixes #757

Todos

Status

Changelog message

Add support for entropy and enthalpy calculations with pymbar4
mikemhenry commented 4 weeks ago

@schuhmc Thanks for this! We missed this API change when we added support for pymbar4 while also supporting pymbar3. Would you mind adding a test? I am also happy to add a test case to check for future regressions.

codecov[bot] commented 4 weeks ago

Codecov Report

Attention: Patch coverage is 54.71698% with 24 lines in your changes missing coverage. Please review.

Project coverage is 84.95%. Comparing base (cba4ed5) to head (9e1d710).

Additional details and impacted files
schuhmc commented 4 weeks ago

Sure, I'll try to implement a test this week if possible.

ijpulidos commented 3 weeks ago

Thanks for the contribution. I agree with the test suggestion, it would be very nice that we have some coverage of this API. If desired, you can just point to us how you are using this API point and we could try to write the test ourselves. Whatever works best for you, @schuhmc .

schuhmc commented 3 weeks ago

I haven't had time to write a test, but I was thinking about basing it on the implementation in pymbar and expand on the already existing harmonic oscillator test case. Additional input is much appreciated. If you'd prefer to write the test yourselves, that's also fine by me. Just let me know.

I don't really use this function, so I don't have a specific use case. It's just something I randomly came across.

schuhmc commented 2 weeks ago

I drafted up a test for get_entropy() and get_enthalpy(). I'm not super confident, but I figured that for the harmonic oscillator the change in the reduced entropy should always be 0 given $U=3/2kT$, $u=U/kT$ -> $u=3/2$ -> $\Delta u = 0$

ijpulidos commented 2 weeks ago

@schuhmc Thanks for writing the test code, this is a great starting point for the tests. I think we should probably write in a separate test, to make it more "unitary". I'm happy to make these changes for the test in a separate PR, of course maintaining your authorship for the changes you've contributed. Does that sound ok?

schuhmc commented 2 weeks ago

@ijpulidos sure, go ahead.