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

OverflowError with Numpy 2 #127

Closed
ehsteve opened this issue Jul 30, 2024 · 3 comments · Fixed by #128
Closed

OverflowError with Numpy 2 #127

ehsteve opened this issue Jul 30, 2024 · 3 comments · Fixed by #128
Assignees
Labels

Comments

@ehsteve
Copy link
Member

ehsteve commented Jul 30, 2024

I've upgraded to Numpy 2.0 and am now encountering the following error. I confirmed that if I downgraded this error goes away so this is definitely a new behavior in the new version of Numpy.

File ~/opt/anaconda3/envs/padre_meddea/lib/python3.10/site-packages/ccsdspy/decode.py:290, in _decode_variable_length(file_bytes, fields)
    288 while offset < len(file_bytes):
    289     packet_starts.append(offset)
--> 290     offset += file_bytes[offset + 4] * 256 + file_bytes[offset + 5] + 7
    292 if offset != len(file_bytes):
    293     missing_bytes = offset - len(file_bytes)

OverflowError: Python integer 256 out of bounds for uint8

I get a similar overflow with FixedLength packets. This may be related to NEP 50.

The easiest way to reproduce these errors is to try to run the test suite with numpy 2. Some of the output is below.

collected 205 items                                                            

ccsdspy/tests/test_byte_order.py FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF [ 19%]
FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF                           [ 41%]
ccsdspy/tests/test_converters.py ..................................FF    [ 59%]
ccsdspy/tests/test_hs.py FFFFFFFFFFFFF                                   [ 65%]
ccsdspy/tests/test_packet_fields.py ......                               [ 68%]
ccsdspy/tests/test_packet_types.py ............FFFFFFFFFFFFFFFFF...F     [ 84%]
ccsdspy/tests/test_primary_header.py .F....                              [ 87%]
ccsdspy/tests/test_regression.py FFFFFFFFF                               [ 91%]
ccsdspy/tests/test_split.py .FF                                          [ 93%]
ccsdspy/tests/test_utils.py .....FFF                                     [ 97%]
ccsdspy/tests/test_var_length.py FFFFFF                                  [100%]

The numpy version should be pinned in the project toml file ASAP because anyone trying to install ccsdspy will run into these errors.

@ehsteve ehsteve added the bug label Jul 30, 2024
@ehsteve
Copy link
Member Author

ehsteve commented Jul 30, 2024

Note that on 30-Jul-2024, the pyproject.toml file was updated to pin the numpy version to <2. Users may run into this bug if they are using Numpy 2. We will work on a new release that supports Numpy 2 soon!

@ddasilva ddasilva self-assigned this Jul 30, 2024
@ehsteve
Copy link
Member Author

ehsteve commented Sep 27, 2024

@ddasilva I did some testing to try to figure out what is the issue here. The primary problem is that the packet data is in bytes which includes the packet length. This packet length value which is also a uint8 is then used to calculate offset values to index in the packet in various places in the code. Calculating packet offsets requires ints and not uint because in some places the values become larger than a uint8 and in others negative packet offsets are used. Since these offsets are frequently initialized with data from the packet (therefore bytes) they are set to uint8 and then the calculations to update the offsets fail.

@ddasilva
Copy link
Collaborator

ddasilva commented Oct 2, 2024

Hi @ehsteve, thanks for your help! I pushed a patch for NumPy 2 in PR #128 (branch name npy2_patch). Can you try it with your code and verify it works before I merge?

The patch should make the code work with both NumPy 1 and 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants