diff --git a/CHANGELOG.md b/CHANGELOG.md index cecccf3..5150951 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,38 @@ against, and semantic-release manages only the **patch** component. - `test_a_copy_does_not_pin_its_source` — asserts the aliasing/copying split directly, via refcount: a view increfs its parent, a copy does not. +### Fixed + +- **`vector()` and `matrix()` no longer silently convert their input.** All + eight factory overloads (native and complex, view and copy) take + `.noconvert()`, so the dtype must match a registered one exactly and the + array must be C-contiguous. + + The reported symptom was unregistered dtypes — `f16`, `uint16`, `uint32`, + `uint64` — coming back as `DenseVector_f32`. Fixing it turned up something + worse, because nanobind's converting pass repacks **layout and dtype + together** and takes the first overload that converts, which is `float32`: + + ```python + a = np.array([0.1, 1.0, 0.2, 2.0, 0.3, 3.0]) # float64 + v = mtl5.vector(a[::2]) + # v is a DenseVector_f32, v.is_view is True, and v[0] == 0.10000000149011612 + ``` + + An ordinary float64 slice was **silently downcast to float32** while + reporting `is_view=True` and aliasing nothing. The old test covering this + used values (1.0, 3.0, 5.0) that are exact in float32, which is why it read + as benign. + + Both now raise `TypeError`, listing every accepted dtype. `np.ascontiguousarray(a)` + is the deliberate way to ask for the copy, and it keeps float64. This matches + what `mtl5.array.asarray` has always done — its `.noconvert()` carries the + same reasoning, and the factories simply never got it. + + **This is a behaviour change.** Code passing a non-contiguous array, or a + dtype outside {f32, f64, i8, i16, i32, i64, u8, c64, c128}, now raises where + it used to return a quietly converted result. + ### Changed - **The build toolchain is pinned**: `cibuildwheel==4.2.0` and `build==1.6.0` in diff --git a/python/src/mtl5_complex.cpp b/python/src/mtl5_complex.cpp index 5ca5e9a..3146ca5 100644 --- a/python/src/mtl5_complex.cpp +++ b/python/src/mtl5_complex.cpp @@ -316,7 +316,15 @@ template void register_complex_factories(nb::module_& m) { m.def("vector", [](nb::ndarray, nb::c_contig, nb::device::cpu> a) { return VectorView(a.shape(0), a.data(), nb::cast(a)); - }, "a"_a, "Create a zero-copy MTL5 vector view of a 1-D complex NumPy array"); + }, nb::arg("a").noconvert(), + "Create a zero-copy MTL5 vector view of a 1-D complex NumPy array.\n\n" + "The view aliases the NumPy buffer; writes through either are visible in both.\n" + "The dtype must be complex64 or complex128 exactly -- whichever this\n" + "overload is for -- and the array must be C-contiguous. Anything else\n" + "raises TypeError rather than converting: a converted array is a view\n" + "of a temporary, so it is neither zero-copy nor the dtype you asked\n" + "for, and a real array would silently acquire an imaginary part. Pass\n" + "np.ascontiguousarray(a), or a.astype(...), to convert deliberately."); m.def("vector_copy", [](nb::ndarray, nb::c_contig, nb::device::cpu> a) { const std::size_t n = a.shape(0); @@ -327,11 +335,27 @@ void register_complex_factories(nb::module_& m) { for (std::size_t i = 0; i < n; ++i) v[i] = src[i]; } return VectorView(std::move(v)); - }, "a"_a, "Create an owning MTL5 vector (copies from a complex NumPy array)"); + }, nb::arg("a").noconvert(), + "Create an owning MTL5 vector (copies from a complex NumPy array).\n\n" + "The dtype must be complex64 or complex128 exactly -- whichever this\n" + "overload is for -- and the array must be C-contiguous. Anything else\n" + "raises TypeError rather than converting. This copies the DATA but not\n" + "the TYPE, so a silent conversion would still hand back a container of\n" + "the wrong precision, and a real array would quietly acquire an\n" + "imaginary part. Pass np.ascontiguousarray(a), or a.astype(...), to\n" + "convert deliberately."); m.def("matrix", [](nb::ndarray, nb::c_contig, nb::device::cpu> a) { return MatrixView(a.shape(0), a.shape(1), a.data(), nb::cast(a)); - }, "a"_a, "Create a zero-copy MTL5 matrix view of a 2-D complex NumPy array"); + }, nb::arg("a").noconvert(), + "Create a zero-copy MTL5 matrix view of a 2-D complex NumPy array.\n\n" + "The view aliases the NumPy buffer; writes through either are visible in both.\n" + "The dtype must be complex64 or complex128 exactly -- whichever this\n" + "overload is for -- and the array must be C-contiguous. Anything else\n" + "raises TypeError rather than converting: a converted array is a view\n" + "of a temporary, so it is neither zero-copy nor the dtype you asked\n" + "for, and a real array would silently acquire an imaginary part. Pass\n" + "np.ascontiguousarray(a), or a.astype(...), to convert deliberately."); m.def("matrix_copy", [](nb::ndarray, nb::c_contig, nb::device::cpu> a) { const std::size_t r = a.shape(0), c = a.shape(1); @@ -344,7 +368,15 @@ void register_complex_factories(nb::module_& m) { M(i, j) = src[i * c + j]; } return MatrixView(std::move(M)); - }, "a"_a, "Create an owning MTL5 matrix (copies from a complex NumPy array)"); + }, nb::arg("a").noconvert(), + "Create an owning MTL5 matrix (copies from a complex NumPy array).\n\n" + "The dtype must be complex64 or complex128 exactly -- whichever this\n" + "overload is for -- and the array must be C-contiguous. Anything else\n" + "raises TypeError rather than converting. This copies the DATA but not\n" + "the TYPE, so a silent conversion would still hand back a container of\n" + "the wrong precision, and a real array would quietly acquire an\n" + "imaginary part. Pass np.ascontiguousarray(a), or a.astype(...), to\n" + "convert deliberately."); } // --------------------------------------------------------------------------- diff --git a/python/src/mtl5_module.cpp b/python/src/mtl5_module.cpp index f9bba6c..e3f8aba 100644 --- a/python/src/mtl5_module.cpp +++ b/python/src/mtl5_module.cpp @@ -225,7 +225,14 @@ void register_native_vector_factory(nb::module_& m) { // Zero-copy: borrow memory from the NumPy array m.def("vector", [](nb::ndarray, nb::c_contig, nb::device::cpu> a) { return VectorView(a.shape(0), a.data(), nb::cast(a)); - }, "a"_a, "Create a zero-copy MTL5 vector view of a 1-D NumPy array"); + }, nb::arg("a").noconvert(), + "Create a zero-copy MTL5 vector view of a 1-D NumPy array.\n\n" + "The dtype must match a registered one exactly and the array must be\n" + "C-contiguous. Anything else raises TypeError rather than converting,\n" + "because a converted array is a view of a temporary: neither zero-copy\n" + "nor the dtype you asked for. A float64 slice such as a[::2] used to\n" + "come back as a float32 vector reporting is_view=True. Pass\n" + "np.ascontiguousarray(a), or a.astype(...), to say so deliberately."); // Explicit copy variant m.def("vector_copy", [](nb::ndarray, nb::c_contig, nb::device::cpu> a) { @@ -238,7 +245,11 @@ void register_native_vector_factory(nb::module_& m) { v[i] = src[i]; } return VectorView(std::move(v)); - }, "a"_a, "Create an owning MTL5 vector (copies data from NumPy array)"); + }, nb::arg("a").noconvert(), + "Create an owning MTL5 vector (copies data from a NumPy array).\n\n" + "Exact dtype and C-contiguity are required here too: this copies the\n" + "data but not the TYPE, so a silent conversion would still hand back a\n" + "vector of the wrong precision."); } // --------------------------------------------------------------------------- @@ -296,7 +307,14 @@ void register_native_matrix_factory(nb::module_& m) { // Zero-copy: borrow memory from the NumPy array m.def("matrix", [](nb::ndarray, nb::c_contig, nb::device::cpu> a) { return MatrixView(a.shape(0), a.shape(1), a.data(), nb::cast(a)); - }, "a"_a, "Create a zero-copy MTL5 matrix view of a 2-D NumPy array"); + }, nb::arg("a").noconvert(), + "Create a zero-copy MTL5 matrix view of a 2-D NumPy array.\n\n" + "The dtype must match a registered one exactly and the array must be\n" + "C-contiguous. Anything else raises TypeError rather than converting,\n" + "because a converted array is a view of a temporary: neither zero-copy\n" + "nor the dtype you asked for. A float64 slice such as a[::2] used to\n" + "come back as a float32 matrix reporting is_view=True. Pass\n" + "np.ascontiguousarray(a), or a.astype(...), to say so deliberately."); // Explicit copy variant m.def("matrix_copy", [](nb::ndarray, nb::c_contig, nb::device::cpu> a) { @@ -310,7 +328,11 @@ void register_native_matrix_factory(nb::module_& m) { M(r, c) = src[r * cols + c]; } return MatrixView(std::move(M)); - }, "a"_a, "Create an owning MTL5 matrix (copies data from NumPy array)"); + }, nb::arg("a").noconvert(), + "Create an owning MTL5 matrix (copies data from a NumPy array).\n\n" + "Exact dtype and C-contiguity are required here too: this copies the\n" + "data but not the TYPE, so a silent conversion would still hand back a\n" + "matrix of the wrong precision."); } // --------------------------------------------------------------------------- diff --git a/tests/test_vector.py b/tests/test_vector.py index c28515d..6052973 100644 --- a/tests/test_vector.py +++ b/tests/test_vector.py @@ -204,19 +204,83 @@ def test_existing_types_keep_their_matmul(self): class TestNonContiguous: - def test_non_contiguous_implicitly_copied(self): - """nanobind implicitly copies non-contiguous arrays to make them contiguous. - The resulting view is NOT shared with the original (safe behavior).""" + """A non-contiguous array is rejected rather than silently repacked. + + This used to be accepted, and what it did was worse than the old docstring + said. nanobind's converting pass repacks layout AND dtype together, and it + takes the first overload that converts — which is float32. So a float64 + slice came back as a *float32* vector, losing precision, while reporting + `is_view=True` and not aliasing anything: + + a = np.array([0.1, 1.0, 0.2, 2.0, 0.3, 3.0]) + mtl5.vector(a[::2])[0] -> 0.10000000149011612 (exact f64: 0.1) + + The values in the old test (1.0, 3.0, 5.0) are exact in float32, which is + why it never noticed. `.noconvert()` on the factories makes this a + TypeError; `np.ascontiguousarray` is the deliberate way to ask for the copy. + """ + + def test_non_contiguous_is_rejected(self): + a = np.array([1.0, 2.0, 3.0, 4.0, 5.0, 6.0]) + with pytest.raises(TypeError): + mtl5.vector(a[::2]) + + def test_ascontiguousarray_is_the_way_through(self): a = np.array([1.0, 2.0, 3.0, 4.0, 5.0, 6.0]) - sliced = a[::2] # non-contiguous: [1, 3, 5] - v = mtl5.vector(sliced) + v = mtl5.vector(np.ascontiguousarray(a[::2])) + assert isinstance(v, mtl5.DenseVector_f64), "and it keeps float64" assert len(v) == 3 - assert v[0] == pytest.approx(1.0) assert v[1] == pytest.approx(3.0) - # Mutation does NOT propagate back to original (implicit copy) + # An explicit copy, so mutation does not reach the source -- same + # end behaviour as before, but now the caller asked for it. v[0] = 99.0 assert a[0] == 1.0 + def test_a_float64_slice_no_longer_becomes_float32(self): + """The regression this closes: silent precision loss on ordinary + slicing. 0.1 is not representable in float32.""" + a = np.array([0.1, 1.0, 0.2, 2.0, 0.3, 3.0], dtype=np.float64) + with pytest.raises(TypeError): + mtl5.vector(a[::2]) + v = mtl5.vector(np.ascontiguousarray(a[::2])) + assert v[0] == 0.1, "float64 must survive the round trip exactly" + + +class TestFactoryRejectsUnregisteredDtypes: + """`.noconvert()` also stops unregistered dtypes being silently converted. + + Before, `mtl5.vector(np.arange(4, dtype=np.float16))` returned a + DenseVector_f32 reporting is_view=True — a view of the converted temporary, + so writes through the NumPy array were invisible. + """ + + @pytest.mark.parametrize( + "dt", + [np.float16, np.uint16, np.uint32, np.uint64], + ids=["f16", "u16", "u32", "u64"], + ) + def test_unregistered_dtype_raises(self, dt): + with pytest.raises(TypeError): + mtl5.vector(np.arange(4, dtype=dt)) + + @pytest.mark.parametrize( + "dt", + [ + np.float32, + np.float64, + np.int8, + np.int16, + np.int32, + np.int64, + np.uint8, + np.complex64, + np.complex128, + ], + ) + def test_registered_dtypes_still_dispatch_exactly(self, dt): + v = mtl5.vector(np.arange(4, dtype=dt)) + assert v.to_numpy().dtype == np.dtype(dt) + class TestNorm: def test_l2_norm_f64(self):