Fix numpy warnings #510
Fix numpy warnings #510
Conversation
|
Matlab_mask is the current issue. I have to replicate legacy behavior. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #510 +/- ##
==========================================
+ Coverage 72.75% 72.77% +0.02%
==========================================
Files 46 46
Lines 6783 6789 +6
Branches 1304 1305 +1
==========================================
+ Hits 4935 4941 +6
Misses 1290 1290
Partials 558 558
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
There is a lot of weirdness in here with the uint8 typing. We should clean this up in the future. For thow this seems to allow the CI to pass. |
| @@ -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 | |||
There was a problem hiding this comment.
This change is not required for this PR but I just added it while I was in the file
| @@ -89,6 +90,9 @@ def matlab_mask(arr: np.ndarray, mask: np.ndarray, diff: Optional[int] = None) - | |||
|
|
|||
| """ | |||
|
|
|||
| if mask.dtype == "uint8": | |||
| 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: | |||
There was a problem hiding this comment.
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.
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| import numpy as np | |||
| from numpy import arcsin, pi, cos, size, array | |||
| from numpy.linalg import linalg | |||
There was a problem hiding this comment.
numpy depricated this import structure for the new one I modified
| @@ -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) | |||
There was a problem hiding this comment.
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.
| @@ -89,6 +90,9 @@ def matlab_mask(arr: np.ndarray, mask: np.ndarray, diff: Optional[int] = None) - | |||
|
|
|||
| """ | |||
|
|
|||
| if mask.dtype == "uint8": | |||
There was a problem hiding this comment.
what if we convert uint8 to uint16 and int8 to int16?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Exaclty, current "value -1 is out of bounds of uint8 type"
There was a problem hiding this comment.
>>> 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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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'There was a problem hiding this comment.
This is all related to this:
numpy/numpy#25639 (comment)
No description provided.