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.

Sign up for GitHub

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

Fix numpy warnings #510

Merged
merged 11 commits into from
Nov 19, 2024
+10 −5
Merged

Fix numpy warnings #510

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2e138d1
update linalg import
waltsims Nov 19, 2024
4437fe2
explicitly cast to intp
waltsims Nov 19, 2024
a073420
remove @typechecker
waltsims Nov 19, 2024
d54606e
cast to int16 to replicate legacy behavior
waltsims Nov 19, 2024
950c04b
update types for larger values
waltsims Nov 19, 2024
3ac8846
try uint16
waltsims Nov 19, 2024
e785e17
convert to uint16
waltsims Nov 19, 2024
87d5d63
revert change
waltsims Nov 19, 2024
4ba0095
test two step
waltsims Nov 19, 2024
3088069
convert uint to unsigned int to allow negative indexing instead of wr…
waltsims Nov 19, 2024
bca3a27
fix type
waltsims Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion kwave/ktransducer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ def delay_mask(self, mode=None):
].min() # -1s compatibility
else:
mask[unflatten_matlab_mask(mask, active_elements_index - 1)] += self.stored_beamforming_delays_offset # -1s compatibility
return mask.astype(np.uint8)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again here for one of the operations, the unit8 type was not being correctly promoted for values larger than 255 so I bumped the type.

return mask.astype(np.uint16)

@property
def elevation_beamforming_delays(self):
Expand Down
2 changes: 1 addition & 1 deletion kwave/utils/kwave_array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import numpy as np
from numpy import arcsin, pi, cos, size, array
from numpy.linalg import linalg
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numpy depricated this import structure for the new one I modified

import numpy.linalg as linalg

from kwave.data import Vector
from kwave.kgrid import kWaveGrid
Expand Down
6 changes: 5 additions & 1 deletion kwave/utils/matlab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from typing import Tuple, Union, Optional, List

from beartype import beartype as typechecker
import numpy as np


Expand Down Expand Up @@ -75,6 +75,7 @@ def matlab_find(arr: Union[List[int], np.ndarray], val: int = 0, mode: str = "ne
return np.expand_dims(arr, -1) # compatibility, n => [n, 1]


@typechecker
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not required for this PR but I just added it while I was in the file

def matlab_mask(arr: np.ndarray, mask: np.ndarray, diff: Optional[int] = None) -> np.ndarray:
"""
Applies a mask to an array and returns the masked elements.
Expand All @@ -89,6 +90,9 @@ def matlab_mask(arr: np.ndarray, mask: np.ndarray, diff: Optional[int] = None) -

"""

if mask.dtype == "uint8":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we convert uint8 to uint16 and int8 to int16?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we promote from uint8 to uint16 we will have the same issue with negative indexing and value wrapping. I have tried it to see.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exaclty, current "value -1 is out of bounds of uint8 type"

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

>>> x
array([0], dtype=uint8)
>>> x - 1
array([255], dtype=uint8)
>>> np.version
<module 'numpy.version' from '/Users/walters/miniconda3/lib/python3.12/site-packages/numpy/version.py'>
>>> np.__version__
'1.26.4'

Here is the legacy behavior

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the new behavior:

>>> import numpy as np
>>> x = np.array([0], dtype='uint8')
>>> x - 1
array([255], dtype=uint8)
>>> np.__version__
'2.1.3'

looks like my assumption was wrong

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be an issue with type promotion with the + operator.

>>> import numpy as np
>>> x = np.array([0], dtype='uint8')
>>> x - 1
array([255], dtype=uint8)
>>> x.ravel(order='F') + -1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: Python integer -1 out of bounds for uint8
>>> x.ravel(order='F').astype("int8") + -1
array([-1], dtype=int8)
>>> x + -1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: Python integer -1 out of bounds for uint8
>>> np.__version__
'2.1.3'

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all related to this:
numpy/numpy#25639 (comment)

mask = mask.astype(np.int16)

if diff is None:
return np.expand_dims(arr.ravel(order="F")[mask.ravel(order="F")], axis=-1) # compatibility, n => [n, 1]
else:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there was an issue with unit8 roll-over to positive indexes. This is solved with negative indexing in python and therefore using int16 is acceptable.

Expand Down
Loading