choderalab / pymbar

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

[DNM] Update fes.py (BUG!) #502

Closed rosadche closed 1 year ago

rosadche commented 1 year ago

This is a one-line code change to mitigate a bug that results in histogramming exiting with a key error. For some reason, histogram bins that actually ARE populated are not correctly being added to the histogram_data dictionary. When the free energy for that bin is meant to be pulled, that bin doesn't exist in the dictionary and the KeyError is thrown. This fix assigns a default value of -1 to the bin_label if the key isn't present nstead of throwing this exception, and thus can allow the user to bypass the troubled bin(s) and still get a free energy surface. This fix does not find the root cause of the issue. I would be happy to provide the example which resulted in me finding the bug, but would require transferring many files.

Lnaden commented 1 year ago

Can you give an example of this we can look at to figure out what happened?

rosadche commented 1 year ago

There is now an example script in the examples subdirectory labeled pull_request_502. Thank you for your time.

Lnaden commented 1 year ago

While I appreciate the example here, this isn't helpful for debugging the problem as you've provided it.

What I need to work on this is a minimalist case where the bug is thrown. Minimal script, minimal input files, etc. Can you provide those as attached ZIP files to this issue rather than as commits to the repository please?

As it stands now, this PR will have to be closed and re-opened as a new one for a few reasons:

If you can give me a minimal script of something here which reproduces the error, preferably without all the extra files, I can debug this, but as it stands I can't advance this right now.

I am going to leave this PR open so it can continue to be discussed, but this PR itself will never be merged. Once we figure out the issue, we'll open a new PR with the fix in it and none of the other files.

Lnaden commented 1 year ago

Closing this as the discussion can continue in #511 and future follow up, especially since this PR cannot be merged as it stands. Feel free to contribute to the conversation there.