choderalab / pymbar

Python implementation of the multistate Bennett acceptance ratio (MBAR)
http://pymbar.readthedocs.io
MIT License
240 stars 93 forks source link

Use self.u_kn when verbose=True in MBAR #539

Closed jaclark5 closed 2 months ago

jaclark5 commented 2 months ago

Resolves #538

Line 296 of mbar.py is changed from: uzero = u_kn[k, indices] - u_kn[l, indices] to uzero = self.u_kn[k, indices] - self.u_kn[l, indices]

mikemhenry commented 2 months ago

@jaclark5 Can you add a test that does the behavior you mention in #538 That will help us prevent any regressions in the future (and might catch some edge case where the numpy array vrs pandas data frame issue crops up)

mrshirts commented 2 months ago

There are some weird cases where one needs either self.u_kn or a copy of it, but this should be totally fine because its just doing some analysis of whether the states are the same or not, not manipulating anything.

jaclark5 commented 2 months ago

@jaclark5 Can you add a test that does the behavior you mention in #538 That will help us prevent any regressions in the future (and might catch some edge case where the numpy array vrs pandas data frame issue crops up)

Sure @mikemhenry, can you help me picture what that would look like? Since model = MBAR.fit() sets self.u_kn = np.array(u_kn, float) I don't foresee a similar issue unless one changes the MBAR.fit() method, as u_kn will be lost and only the attribute, model.u_nk will be pertinent to other methods.

mikemhenry commented 2 months ago

@jaclark5 you are right!

mikemhenry commented 2 months ago

The linting errors are happening on master, so this one is good to merge in!