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

add entropy/enthalpy support for pymbar4 #758

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

schuhmc
Copy link

@schuhmc schuhmc commented Oct 28, 2024

Description

Fixes #757

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

  • Ready to go

Changelog message

Add support for entropy and enthalpy calculations with pymbar4

@mikemhenry
Copy link
Contributor

@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.

Copy link

codecov bot commented Oct 28, 2024

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
Copy link
Author

schuhmc commented Oct 28, 2024

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

@ijpulidos
Copy link
Contributor

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
Copy link
Author

schuhmc commented Nov 5, 2024

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
Copy link
Author

schuhmc commented Nov 6, 2024

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
Copy link
Contributor

ijpulidos commented Nov 6, 2024

@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
Copy link
Author

schuhmc commented Nov 7, 2024

@ijpulidos sure, go ahead.

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.

get_entropy() and get_enthalpy() does not work with pymbar4
3 participants