Skip to content

Commit d161443

Browse files
kwagyemankwagyeman
authored
ndarray: Fix four bugs in the modulo operator (% and %=) (#749)
* ndarray: fix binary modulo result dtype for uint16 % uint8 ndarray_binary_modulo allocated the result array as NDARRAY_UINT8 but then looped with uint16_t arithmetic via BINARY_LOOP. Because BINARY_LOOP writes through a uint16_t pointer but the array's itemsize is 1 byte, this caused out-of-bounds writes that corrupted both the result values and adjacent memory. The result dtype must be NDARRAY_UINT16 to match the left operand and to give BINARY_LOOP the correct element size. * ndarray: fix ndarray_inplace_modulo for all dtype combinations Four bugs in the inplace modulo implementation: 1. ulab.h defined NDARRAY_HAS_INPLACE_MODU (typo, missing "LO"), so the NDARRAY_HAS_INPLACE_MODULO feature guard was never set and the entire ndarray_inplace_modulo function was compiled out. All %= operations silently fell back to binary % + rebind. 2. The function referenced larray and rarray in every INLINE_MODULO_FLOAT_LOOP call but never declared them, which would have been a compile error once the typo above was fixed. 3. The second dtype branch checked NDARRAY_UINT8 again instead of NDARRAY_INT8, making float %= int8 a no-op. 4. Only float lhs was handled; integer lhs returned the array unchanged instead of performing in-place integer modulo. Fix all four: correct the feature-guard typo, add the larray/rarray declarations, fix the NDARRAY_INT8 branch, and add full integer dispatch using INPLACE_LOOP with %=. The float path continues to use fmod via INLINE_MODULO_FLOAT_LOOP since C does not support %= on floating-point types. Integer lhs with float rhs raises TypeError, matching NumPy's in-place casting semantics. * tests: update modulo tests to cover fixed bugs - Add try/except TypeError around the inplace loop: integer lhs %= float rhs now raises TypeError (matching NumPy casting semantics) rather than silently rebinding via binary % fallback. - Update modulo.py.exp: binary uint16 % uint8 now shows correct values and dtype=uint16; inplace section now shows stable dtypes (true in-place for integer types) and the four expected TypeErrors. - Add a regression section with targeted cases for each of the four bugs: binary uint16 % uint8 dtype, inplace integer types, and float %= int8.
1 parent 2a94125 commit d161443

4 files changed

Lines changed: 126 additions & 37 deletions

File tree

‎code/ndarray_operators.c‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ mp_obj_t ndarray_binary_modulo(ndarray_obj_t *lhs, ndarray_obj_t *rhs,
292292
}
293293
} else if(lhs->dtype == NDARRAY_UINT16) {
294294
if(rhs->dtype == NDARRAY_UINT8) {
295-
results = ndarray_new_dense_ndarray(ndim, shape, NDARRAY_UINT8);
295+
results = ndarray_new_dense_ndarray(ndim, shape, NDARRAY_UINT16);
296296
BINARY_LOOP(results, uint16_t, uint16_t, uint8_t, larray, lstrides, rarray, rstrides, %);
297297
} else if(rhs->dtype == NDARRAY_INT8) {
298298
results = ndarray_new_dense_ndarray(ndim, shape, NDARRAY_FLOAT);
@@ -1177,12 +1177,15 @@ mp_obj_t ndarray_inplace_ams(ndarray_obj_t *lhs, ndarray_obj_t *rhs, int32_t *rs
11771177
#if NDARRAY_HAS_INPLACE_MODULO
11781178
mp_obj_t ndarray_inplace_modulo(ndarray_obj_t *lhs, ndarray_obj_t *rhs, int32_t *rstrides) {
11791179
if((lhs->dtype != NDARRAY_FLOAT) && (rhs->dtype == NDARRAY_FLOAT)) {
1180-
mp_raise_TypeError(MP_ERROR_TEXT("results cannot be cast to specified type"));
1180+
mp_raise_TypeError(MP_ERROR_TEXT("cannot cast output with casting rule"));
11811181
}
1182+
uint8_t *larray = (uint8_t *)lhs->array;
1183+
uint8_t *rarray = (uint8_t *)rhs->array;
11821184
if(lhs->dtype == NDARRAY_FLOAT) {
1185+
// Float lhs: use fmod since C does not support %= for floating-point types.
11831186
if(rhs->dtype == NDARRAY_UINT8) {
11841187
INLINE_MODULO_FLOAT_LOOP(lhs, uint8_t, larray, rarray, rstrides);
1185-
} else if(rhs->dtype == NDARRAY_UINT8) {
1188+
} else if(rhs->dtype == NDARRAY_INT8) {
11861189
INLINE_MODULO_FLOAT_LOOP(lhs, int8_t, larray, rarray, rstrides);
11871190
} else if(rhs->dtype == NDARRAY_UINT16) {
11881191
INLINE_MODULO_FLOAT_LOOP(lhs, uint16_t, larray, rarray, rstrides);
@@ -1191,6 +1194,46 @@ mp_obj_t ndarray_inplace_modulo(ndarray_obj_t *lhs, ndarray_obj_t *rhs, int32_t
11911194
} else {
11921195
INLINE_MODULO_FLOAT_LOOP(lhs, mp_float_t, larray, rarray, rstrides);
11931196
}
1197+
} else if(lhs->dtype == NDARRAY_UINT8) {
1198+
if(rhs->dtype == NDARRAY_UINT8) {
1199+
INPLACE_LOOP(lhs, uint8_t, uint8_t, larray, rarray, rstrides, %=);
1200+
} else if(rhs->dtype == NDARRAY_INT8) {
1201+
INPLACE_LOOP(lhs, uint8_t, int8_t, larray, rarray, rstrides, %=);
1202+
} else if(rhs->dtype == NDARRAY_UINT16) {
1203+
INPLACE_LOOP(lhs, uint8_t, uint16_t, larray, rarray, rstrides, %=);
1204+
} else {
1205+
INPLACE_LOOP(lhs, uint8_t, int16_t, larray, rarray, rstrides, %=);
1206+
}
1207+
} else if(lhs->dtype == NDARRAY_INT8) {
1208+
if(rhs->dtype == NDARRAY_UINT8) {
1209+
INPLACE_LOOP(lhs, int8_t, uint8_t, larray, rarray, rstrides, %=);
1210+
} else if(rhs->dtype == NDARRAY_INT8) {
1211+
INPLACE_LOOP(lhs, int8_t, int8_t, larray, rarray, rstrides, %=);
1212+
} else if(rhs->dtype == NDARRAY_UINT16) {
1213+
INPLACE_LOOP(lhs, int8_t, uint16_t, larray, rarray, rstrides, %=);
1214+
} else {
1215+
INPLACE_LOOP(lhs, int8_t, int16_t, larray, rarray, rstrides, %=);
1216+
}
1217+
} else if(lhs->dtype == NDARRAY_UINT16) {
1218+
if(rhs->dtype == NDARRAY_UINT8) {
1219+
INPLACE_LOOP(lhs, uint16_t, uint8_t, larray, rarray, rstrides, %=);
1220+
} else if(rhs->dtype == NDARRAY_INT8) {
1221+
INPLACE_LOOP(lhs, uint16_t, int8_t, larray, rarray, rstrides, %=);
1222+
} else if(rhs->dtype == NDARRAY_UINT16) {
1223+
INPLACE_LOOP(lhs, uint16_t, uint16_t, larray, rarray, rstrides, %=);
1224+
} else {
1225+
INPLACE_LOOP(lhs, uint16_t, int16_t, larray, rarray, rstrides, %=);
1226+
}
1227+
} else if(lhs->dtype == NDARRAY_INT16) {
1228+
if(rhs->dtype == NDARRAY_UINT8) {
1229+
INPLACE_LOOP(lhs, int16_t, uint8_t, larray, rarray, rstrides, %=);
1230+
} else if(rhs->dtype == NDARRAY_INT8) {
1231+
INPLACE_LOOP(lhs, int16_t, int8_t, larray, rarray, rstrides, %=);
1232+
} else if(rhs->dtype == NDARRAY_UINT16) {
1233+
INPLACE_LOOP(lhs, int16_t, uint16_t, larray, rarray, rstrides, %=);
1234+
} else {
1235+
INPLACE_LOOP(lhs, int16_t, int16_t, larray, rarray, rstrides, %=);
1236+
}
11941237
}
11951238
return MP_OBJ_FROM_PTR(lhs);
11961239
}

‎code/ulab.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@
166166
#endif
167167

168168
#ifndef NDARRAY_HAS_INPLACE_MODULO
169-
#define NDARRAY_HAS_INPLACE_MODU (1)
169+
#define NDARRAY_HAS_INPLACE_MODULO (1)
170170
#endif
171171

172172
#ifndef NDARRAY_HAS_INPLACE_MULTIPLY

‎tests/2d/numpy/modulo.py‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,49 @@
1818
print('=' * 30)
1919
print()
2020

21+
# integer lhs %= float rhs raises TypeError (cannot store float result in integer array),
22+
# matching NumPy's casting semantics for in-place operations.
2123
for dtype1 in dtypes:
2224
x1 = np.array(range(6), dtype=dtype1).reshape((2, 3))
2325
for dtype2 in dtypes:
2426
x2 = np.array(range(1, 4), dtype=dtype2)
25-
x1 %= x2
26-
print(x1)
27+
try:
28+
x1 %= x2
29+
print(x1)
30+
except TypeError:
31+
print('TypeError')
32+
33+
print()
34+
print('=' * 30)
35+
print('regression tests')
36+
print('=' * 30)
37+
print()
38+
39+
# Bug: binary uint16 % uint8 was returning dtype=uint8 with corrupted values.
40+
# Result dtype must be uint16 and values must be correct.
41+
a = np.array([0, 1, 2, 3, 4, 5], dtype=np.uint16)
42+
b = np.array([1, 2, 3, 1, 2, 3], dtype=np.uint8)
43+
r = a % b
44+
print(r)
45+
46+
# Bug: inplace_modulo was disabled by a typo in the feature-guard macro
47+
# (NDARRAY_HAS_INPLACE_MODU instead of NDARRAY_HAS_INPLACE_MODULO), so %=
48+
# silently fell back to binary % for all types. Verify it now works in-place
49+
# for integer types without changing the array dtype.
50+
a = np.array([3, 4, 5], dtype=np.uint8)
51+
a %= np.array([2, 3, 4], dtype=np.uint8)
52+
print(a)
53+
54+
a = np.array([3, 4, 5], dtype=np.int8)
55+
a %= np.array([2, 3, 4], dtype=np.int8)
56+
print(a)
57+
58+
a = np.array([3, 4, 5], dtype=np.uint16)
59+
a %= np.array([2, 3, 4], dtype=np.uint8)
60+
print(a)
61+
62+
# Bug: inplace float %= int8 was a no-op because the second branch checked
63+
# NDARRAY_UINT8 again instead of NDARRAY_INT8. Verify fmod is applied.
64+
a = np.array([3.5, 4.5, 5.5])
65+
a %= np.array([2, 3, 4], dtype=np.int8)
66+
print(a)

‎tests/2d/numpy/modulo.py.exp‎

Lines changed: 37 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ array([[0, 1, 2],
1818
[0, 0, 2]], dtype=int16)
1919
array([[0.0, 1.0, 2.0],
2020
[0.0, 0.0, 2.0]], dtype=float64)
21-
array([[0, 0, 1],
22-
[0, 2, 0]], dtype=uint8)
21+
array([[0, 1, 2],
22+
[0, 0, 2]], dtype=uint16)
2323
array([[0.0, 1.0, 2.0],
2424
[0.0, 0.0, 2.0]], dtype=float64)
2525
array([[0, 1, 2],
@@ -56,37 +56,39 @@ inplace modulo
5656
array([[0, 1, 2],
5757
[0, 0, 2]], dtype=uint8)
5858
array([[0, 1, 2],
59-
[0, 0, 2]], dtype=int16)
60-
array([[0.0, 1.0, 2.0],
61-
[0.0, 0.0, 2.0]], dtype=float64)
62-
array([[0.0, 1.0, 2.0],
63-
[0.0, 0.0, 2.0]], dtype=float64)
64-
array([[0.0, 1.0, 2.0],
65-
[0.0, 0.0, 2.0]], dtype=float64)
59+
[0, 0, 2]], dtype=uint8)
60+
array([[0, 1, 2],
61+
[0, 0, 2]], dtype=uint8)
62+
array([[0, 1, 2],
63+
[0, 0, 2]], dtype=uint8)
64+
TypeError
65+
array([[0, 1, 2],
66+
[0, 0, 2]], dtype=int8)
67+
array([[0, 1, 2],
68+
[0, 0, 2]], dtype=int8)
69+
array([[0, 1, 2],
70+
[0, 0, 2]], dtype=int8)
71+
array([[0, 1, 2],
72+
[0, 0, 2]], dtype=int8)
73+
TypeError
74+
array([[0, 1, 2],
75+
[0, 0, 2]], dtype=uint16)
76+
array([[0, 1, 2],
77+
[0, 0, 2]], dtype=uint16)
78+
array([[0, 1, 2],
79+
[0, 0, 2]], dtype=uint16)
80+
array([[0, 1, 2],
81+
[0, 0, 2]], dtype=uint16)
82+
TypeError
6683
array([[0, 1, 2],
6784
[0, 0, 2]], dtype=int16)
6885
array([[0, 1, 2],
6986
[0, 0, 2]], dtype=int16)
70-
array([[0.0, 1.0, 2.0],
71-
[0.0, 0.0, 2.0]], dtype=float64)
72-
array([[0.0, 1.0, 2.0],
73-
[0.0, 0.0, 2.0]], dtype=float64)
74-
array([[0.0, 1.0, 2.0],
75-
[0.0, 0.0, 2.0]], dtype=float64)
76-
array([[0, 0, 1],
77-
[0, 2, 0]], dtype=uint8)
78-
array([[0, 0, 1],
79-
[0, 0, 0]], dtype=int16)
80-
array([[0.0, 0.0, 1.0],
81-
[0.0, 0.0, 0.0]], dtype=float64)
82-
array([[0.0, 0.0, 1.0],
83-
[0.0, 0.0, 0.0]], dtype=float64)
84-
array([[0.0, 0.0, 1.0],
85-
[0.0, 0.0, 0.0]], dtype=float64)
8687
array([[0, 1, 2],
8788
[0, 0, 2]], dtype=int16)
8889
array([[0, 1, 2],
8990
[0, 0, 2]], dtype=int16)
91+
TypeError
9092
array([[0.0, 1.0, 2.0],
9193
[0.0, 0.0, 2.0]], dtype=float64)
9294
array([[0.0, 1.0, 2.0],
@@ -97,9 +99,13 @@ array([[0.0, 1.0, 2.0],
9799
[0.0, 0.0, 2.0]], dtype=float64)
98100
array([[0.0, 1.0, 2.0],
99101
[0.0, 0.0, 2.0]], dtype=float64)
100-
array([[0.0, 1.0, 2.0],
101-
[0.0, 0.0, 2.0]], dtype=float64)
102-
array([[0.0, 1.0, 2.0],
103-
[0.0, 0.0, 2.0]], dtype=float64)
104-
array([[0.0, 1.0, 2.0],
105-
[0.0, 0.0, 2.0]], dtype=float64)
102+
103+
==============================
104+
regression tests
105+
==============================
106+
107+
array([0, 1, 2, 0, 0, 2], dtype=uint16)
108+
array([1, 1, 1], dtype=uint8)
109+
array([1, 1, 1], dtype=int8)
110+
array([1, 1, 1], dtype=uint16)
111+
array([1.5, 1.5, 1.5], dtype=float64)

0 commit comments

Comments
 (0)