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

Fixing OverflowError: Python integer -1000 out of bounds for uint32 #158

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

gmrandazzo
Copy link

This patch fix #157

  • This code checks if the input int32 is equal to 4294967295 (which is 2^32 - 1, the maximum value for a 32-bit unsigned integer). If true, it returns -1 like your implementation.

Then here it comes the difference with your implementation:

  • uint32_value = np.uint32(int32) converts int32 to a NumPy unsigned 32-bit integer type.
  • Then checks if the unsigned value is greater than or equal to 2^31 (2147483648). If true, it means the value is in the upper half of the 32-bit unsigned integer range. It subtracts 2^32 (4294967296) from it and returns the result as an integer.
  • Else, if the value is less than 2^31, it simply returns the original input.

It is simply a more efficient way.

Here the outcome.

image

…nt32_to_negative(int32) in supplemental.py
@gmrandazzo
Copy link
Author

Ah!! One last comment: It would be good to add a unit test for this function, something like the ligand you are talking about, something that you have found in some PDB.
Then... nice piece of software... :)

@sorui-qin
Copy link

Hi Giuseppe,

I encountered the OverflowError as well, and initially, I thought it might be an issue with the pdb files I was using. However, after testing them on the online PLIP server, I found that there was nothing wrong. It left me quite confused.

Fortunately, I fixed supplemental.py as you suggested, and it worked! I really appreciate your help, and I have recommended that the repository owner address this issue.

@gmrandazzo
Copy link
Author

I installed the latest version and the bug is not there anymore. However, i still maintain the pull request open since this int32_to_negative conversion runs in O(1) constant time and the current implementation O(n) where n is the range of negative numbers being pre-computed and looks less efficient since every function call is creating a dictionary and iterating through a range of -1000 to -1.

@snbolz
Copy link

snbolz commented Dec 13, 2024

Hi Giuseppe,
thanks a lot for your suggestion to make the code more efficient. From what I understand, the function is necessary solely because OBResidue::GetNum() returns unsigned integers, but ligand positions can be negative. Therefore, I would suggest changing the function's name to clarify its intention.

With your solution, the special case workaround would not be required anymore. Moreover, I believe NumPy is not needed. I would suggest testing for the positive numbers first and only converting the number if it is >= 2^31. Additionally, using 2 ** 32 or 1 << 32 (or 2 ** 31 or 1 << 31 for 2147483648) is less error-prone than hard-coding the numbers, and it increases readability.

Based on your solution, I suggest the following:
def convert_to_signed_int(res_num):
"""Converts residue numbers (OBResidue::GetNum()) >= 2147483648 into their two's complement signed counterparts
(negative numbers)."""
    if res_num < (2 ** 31):
        return res_num
    return int(res_num - (2 ** 32))

Furthermore, I think the function call in line 352 of preparation.py is redundant. Lines 350-352 could be removed:
# Check if a negative residue number is represented as a 32 bit integer
if rnum > 10 ** 5:
rnum = int32_to_negative(rnum)

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.

3 participants