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

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

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

jaclark5
Copy link
Contributor

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

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

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

jaclark5 commented Aug 28, 2024

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

@jaclark5 you are right!

@mikemhenry
Copy link
Contributor

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

@mikemhenry mikemhenry merged commit 76664b7 into choderalab:master Aug 28, 2024
16 of 17 checks passed
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.

Verbose=True with MBAR results in an error for alternative input types of u_kn
3 participants