From 6d98d4d8d4682d3f4ea405d1f800bf5427f5cd75 Mon Sep 17 00:00:00 2001 From: gentlegiantJGC Date: Mon, 11 Nov 2024 22:51:01 +0000 Subject: [PATCH 1/6] Add type hints for args and kwargs (#5357) * Allow subclasses of args and kwargs The current implementation disallows subclasses of args and kwargs * Added object type hint to args and kwargs * Added type hinted args and kwargs classes * Changed default type hint to typing.Any * Removed args and kwargs type hint * Updated tests Modified the tests from #5381 to use the real Args and KWArgs classes * Added comment --- include/pybind11/cast.h | 8 ++++++++ include/pybind11/pytypes.h | 12 ++++++++++++ tests/test_kwargs_and_defaults.cpp | 22 +--------------------- tests/test_kwargs_and_defaults.py | 2 +- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index f6a7e83be..2ae25c2eb 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1012,10 +1012,18 @@ template <> struct handle_type_name { static constexpr auto name = const_name("*args"); }; +template +struct handle_type_name> { + static constexpr auto name = const_name("*args: ") + make_caster::name; +}; template <> struct handle_type_name { static constexpr auto name = const_name("**kwargs"); }; +template +struct handle_type_name> { + static constexpr auto name = const_name("**kwargs: ") + make_caster::name; +}; template <> struct handle_type_name { static constexpr auto name = const_name(); diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 027e36098..60d51fdcf 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -2226,6 +2226,18 @@ class kwargs : public dict { PYBIND11_OBJECT_DEFAULT(kwargs, dict, PyDict_Check) }; +// Subclasses of args and kwargs to support type hinting +// as defined in PEP 484. See #5357 for more info. +template +class Args : public args { + using args::args; +}; + +template +class KWArgs : public kwargs { + using kwargs::kwargs; +}; + class anyset : public object { public: PYBIND11_OBJECT(anyset, object, PyAnySet_Check) diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index 09036ccd5..831947f16 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -14,26 +14,6 @@ #include -// Classes needed for subclass test. -class ArgsSubclass : public py::args { - using py::args::args; -}; -class KWArgsSubclass : public py::kwargs { - using py::kwargs::kwargs; -}; -namespace pybind11 { -namespace detail { -template <> -struct handle_type_name { - static constexpr auto name = const_name("*Args"); -}; -template <> -struct handle_type_name { - static constexpr auto name = const_name("**KWArgs"); -}; -} // namespace detail -} // namespace pybind11 - TEST_SUBMODULE(kwargs_and_defaults, m) { auto kw_func = [](int x, int y) { return "x=" + std::to_string(x) + ", y=" + std::to_string(y); }; @@ -345,7 +325,7 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { // Test support for args and kwargs subclasses m.def("args_kwargs_subclass_function", - [](const ArgsSubclass &args, const KWArgsSubclass &kwargs) { + [](const py::Args &args, const py::KWArgs &kwargs) { return py::make_tuple(args, kwargs); }); } diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py index e3e9a0a0d..e558d8ad2 100644 --- a/tests/test_kwargs_and_defaults.py +++ b/tests/test_kwargs_and_defaults.py @@ -20,7 +20,7 @@ def test_function_signatures(doc): ) assert ( doc(m.args_kwargs_subclass_function) - == "args_kwargs_subclass_function(*Args, **KWArgs) -> tuple" + == "args_kwargs_subclass_function(*args: str, **kwargs: str) -> tuple" ) assert ( doc(m.KWClass.foo0) From 7f94f24d6467f14276c9f4e72fae09a364aa0b6c Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Tue, 12 Nov 2024 07:35:28 +0800 Subject: [PATCH 2/6] feat(typing): allow annotate methods with `pos_only` when only have the `self` argument (#5403) * feat: allow annotate methods with `pos_only` when only have the `self` argument * chore(typing): make arguments for auto-generated dunder methods positional-only * docs: add more comments to improve readability * style: fix nit suggestions * Add test_self_only_pos_only() in tests/test_methods_and_attributes * test: add docstring tests for generated dunder methods * test: remove failed tests * fix(test): run `gc.collect()` three times for refcount tests --------- Co-authored-by: Ralf W. Grosse-Kunstleve --- include/pybind11/detail/init.h | 2 +- include/pybind11/pybind11.h | 59 ++++++++++++++++++-------- tests/conftest.py | 3 +- tests/test_enum.py | 60 +++++++++++++++++++++++++++ tests/test_methods_and_attributes.cpp | 2 +- tests/test_methods_and_attributes.py | 7 ++++ tests/test_pickling.py | 17 ++++++++ tests/test_sequences_and_iterators.py | 30 +++++++++++--- 8 files changed, 155 insertions(+), 25 deletions(-) diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index 79cc930c8..3eeeaaf97 100644 --- a/include/pybind11/detail/init.h +++ b/include/pybind11/detail/init.h @@ -410,7 +410,7 @@ struct pickle_factory { template void execute(Class &cl, const Extra &...extra) && { - cl.def("__getstate__", std::move(get)); + cl.def("__getstate__", std::move(get), pos_only()); #if defined(PYBIND11_CPP14) cl.def( diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b4f93f1a6..fe8384e57 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -301,9 +301,20 @@ protected: constexpr bool has_kw_only_args = any_of...>::value, has_pos_only_args = any_of...>::value, has_arg_annotations = any_of...>::value; + constexpr bool has_is_method = any_of...>::value; + // The implicit `self` argument is not present and not counted in method definitions. + constexpr bool has_args = cast_in::args_pos >= 0; + constexpr bool is_method_with_self_arg_only = has_is_method && !has_args; static_assert(has_arg_annotations || !has_kw_only_args, "py::kw_only requires the use of argument annotations"); - static_assert(has_arg_annotations || !has_pos_only_args, + static_assert(((/* Need `py::arg("arg_name")` annotation in function/method. */ + has_arg_annotations) + || (/* Allow methods with no arguments `def method(self, /): ...`. + * A method has at least one argument `self`. There can be no + * `py::arg` annotation. E.g. `class.def("method", py::pos_only())`. + */ + is_method_with_self_arg_only)) + || !has_pos_only_args, "py::pos_only requires the use of argument annotations (for docstrings " "and aligning the annotations to the argument)"); @@ -2022,9 +2033,11 @@ struct enum_base { .format(std::move(type_name), enum_name(arg), int_(arg)); }, name("__repr__"), - is_method(m_base)); + is_method(m_base), + pos_only()); - m_base.attr("name") = property(cpp_function(&enum_name, name("name"), is_method(m_base))); + m_base.attr("name") + = property(cpp_function(&enum_name, name("name"), is_method(m_base), pos_only())); m_base.attr("__str__") = cpp_function( [](handle arg) -> str { @@ -2032,7 +2045,8 @@ struct enum_base { return pybind11::str("{}.{}").format(std::move(type_name), enum_name(arg)); }, name("__str__"), - is_method(m_base)); + is_method(m_base), + pos_only()); if (options::show_enum_members_docstring()) { m_base.attr("__doc__") = static_property( @@ -2087,7 +2101,8 @@ struct enum_base { }, \ name(op), \ is_method(m_base), \ - arg("other")) + arg("other"), \ + pos_only()) #define PYBIND11_ENUM_OP_CONV(op, expr) \ m_base.attr(op) = cpp_function( \ @@ -2097,7 +2112,8 @@ struct enum_base { }, \ name(op), \ is_method(m_base), \ - arg("other")) + arg("other"), \ + pos_only()) #define PYBIND11_ENUM_OP_CONV_LHS(op, expr) \ m_base.attr(op) = cpp_function( \ @@ -2107,7 +2123,8 @@ struct enum_base { }, \ name(op), \ is_method(m_base), \ - arg("other")) + arg("other"), \ + pos_only()) if (is_convertible) { PYBIND11_ENUM_OP_CONV_LHS("__eq__", !b.is_none() && a.equal(b)); @@ -2127,7 +2144,8 @@ struct enum_base { m_base.attr("__invert__") = cpp_function([](const object &arg) { return ~(int_(arg)); }, name("__invert__"), - is_method(m_base)); + is_method(m_base), + pos_only()); } } else { PYBIND11_ENUM_OP_STRICT("__eq__", int_(a).equal(int_(b)), return false); @@ -2147,11 +2165,15 @@ struct enum_base { #undef PYBIND11_ENUM_OP_CONV #undef PYBIND11_ENUM_OP_STRICT - m_base.attr("__getstate__") = cpp_function( - [](const object &arg) { return int_(arg); }, name("__getstate__"), is_method(m_base)); + m_base.attr("__getstate__") = cpp_function([](const object &arg) { return int_(arg); }, + name("__getstate__"), + is_method(m_base), + pos_only()); - m_base.attr("__hash__") = cpp_function( - [](const object &arg) { return int_(arg); }, name("__hash__"), is_method(m_base)); + m_base.attr("__hash__") = cpp_function([](const object &arg) { return int_(arg); }, + name("__hash__"), + is_method(m_base), + pos_only()); } PYBIND11_NOINLINE void value(char const *name_, object value, const char *doc = nullptr) { @@ -2243,9 +2265,9 @@ public: m_base.init(is_arithmetic, is_convertible); def(init([](Scalar i) { return static_cast(i); }), arg("value")); - def_property_readonly("value", [](Type value) { return (Scalar) value; }); - def("__int__", [](Type value) { return (Scalar) value; }); - def("__index__", [](Type value) { return (Scalar) value; }); + def_property_readonly("value", [](Type value) { return (Scalar) value; }, pos_only()); + def("__int__", [](Type value) { return (Scalar) value; }, pos_only()); + def("__index__", [](Type value) { return (Scalar) value; }, pos_only()); attr("__setstate__") = cpp_function( [](detail::value_and_holder &v_h, Scalar arg) { detail::initimpl::setstate( @@ -2254,7 +2276,8 @@ public: detail::is_new_style_constructor(), pybind11::name("__setstate__"), is_method(*this), - arg("state")); + arg("state"), + pos_only()); } /// Export enumeration entries into the parent scope @@ -2440,7 +2463,8 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) { if (!detail::get_type_info(typeid(state), false)) { class_(handle(), "iterator", pybind11::module_local()) - .def("__iter__", [](state &s) -> state & { return s; }) + .def( + "__iter__", [](state &s) -> state & { return s; }, pos_only()) .def( "__next__", [](state &s) -> ValueType { @@ -2457,6 +2481,7 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) { // NOLINTNEXTLINE(readability-const-return-type) // PR #3263 }, std::forward(extra)..., + pos_only(), Policy); } diff --git a/tests/conftest.py b/tests/conftest.py index 9e7ca8812..8eb803d2f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -198,10 +198,11 @@ def pytest_assertrepr_compare(op, left, right): # noqa: ARG001 def gc_collect(): - """Run the garbage collector twice (needed when running + """Run the garbage collector three times (needed when running reference counting tests with PyPy)""" gc.collect() gc.collect() + gc.collect() def pytest_configure(): diff --git a/tests/test_enum.py b/tests/test_enum.py index 03cd1c1a6..044ef1803 100644 --- a/tests/test_enum.py +++ b/tests/test_enum.py @@ -1,6 +1,8 @@ # ruff: noqa: SIM201 SIM300 SIM202 from __future__ import annotations +import re + import pytest import env # noqa: F401 @@ -271,3 +273,61 @@ def test_docstring_signatures(): def test_str_signature(): for enum_type in [m.ScopedEnum, m.UnscopedEnum]: assert enum_type.__str__.__doc__.startswith("__str__") + + +def test_generated_dunder_methods_pos_only(): + for enum_type in [m.ScopedEnum, m.UnscopedEnum]: + for binary_op in [ + "__eq__", + "__ne__", + "__ge__", + "__gt__", + "__lt__", + "__le__", + "__and__", + "__rand__", + # "__or__", # fail with some compilers (__doc__ = "Return self|value.") + # "__ror__", # fail with some compilers (__doc__ = "Return value|self.") + "__xor__", + "__rxor__", + "__rxor__", + ]: + method = getattr(enum_type, binary_op, None) + if method is not None: + assert ( + re.match( + rf"^{binary_op}\(self: [\w\.]+, other: [\w\.]+, /\)", + method.__doc__, + ) + is not None + ) + for unary_op in [ + "__int__", + "__index__", + "__hash__", + "__str__", + "__repr__", + ]: + method = getattr(enum_type, unary_op, None) + if method is not None: + assert ( + re.match( + rf"^{unary_op}\(self: [\w\.]+, /\)", + method.__doc__, + ) + is not None + ) + assert ( + re.match( + r"^__getstate__\(self: [\w\.]+, /\)", + enum_type.__getstate__.__doc__, + ) + is not None + ) + assert ( + re.match( + r"^__setstate__\(self: [\w\.]+, state: [\w\.]+, /\)", + enum_type.__setstate__.__doc__, + ) + is not None + ) diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index f433847c7..e324c8bdd 100644 --- a/tests/test_methods_and_attributes.cpp +++ b/tests/test_methods_and_attributes.cpp @@ -294,7 +294,7 @@ TEST_SUBMODULE(methods_and_attributes, m) { static_cast( &ExampleMandA::overloaded)); }) - .def("__str__", &ExampleMandA::toString) + .def("__str__", &ExampleMandA::toString, py::pos_only()) .def_readwrite("value", &ExampleMandA::value); // test_copy_method diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 91c7b7751..cecc18464 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -19,6 +19,13 @@ NO_DELETER_MSG = ( ) +def test_self_only_pos_only(): + assert ( + m.ExampleMandA.__str__.__doc__ + == "__str__(self: pybind11_tests.methods_and_attributes.ExampleMandA, /) -> str\n" + ) + + def test_methods_and_attributes(): instance1 = m.ExampleMandA() instance2 = m.ExampleMandA(32) diff --git a/tests/test_pickling.py b/tests/test_pickling.py index 3e2e45396..d3551efc1 100644 --- a/tests/test_pickling.py +++ b/tests/test_pickling.py @@ -93,3 +93,20 @@ def test_roundtrip_simple_cpp_derived(): # Issue #3062: pickleable base C++ classes can incur object slicing # if derived typeid is not registered with pybind11 assert not m.check_dynamic_cast_SimpleCppDerived(p2) + + +def test_new_style_pickle_getstate_pos_only(): + assert ( + re.match( + r"^__getstate__\(self: [\w\.]+, /\)", m.PickleableNew.__getstate__.__doc__ + ) + is not None + ) + if hasattr(m, "PickleableWithDictNew"): + assert ( + re.match( + r"^__getstate__\(self: [\w\.]+, /\)", + m.PickleableWithDictNew.__getstate__.__doc__, + ) + is not None + ) diff --git a/tests/test_sequences_and_iterators.py b/tests/test_sequences_and_iterators.py index 20abd29d5..6fba6fba3 100644 --- a/tests/test_sequences_and_iterators.py +++ b/tests/test_sequences_and_iterators.py @@ -1,5 +1,7 @@ from __future__ import annotations +import re + import pytest from pytest import approx # noqa: PT013 @@ -253,16 +255,12 @@ def test_python_iterator_in_cpp(): def test_iterator_passthrough(): """#181: iterator passthrough did not compile""" - from pybind11_tests.sequences_and_iterators import iterator_passthrough - values = [3, 5, 7, 9, 11, 13, 15] - assert list(iterator_passthrough(iter(values))) == values + assert list(m.iterator_passthrough(iter(values))) == values def test_iterator_rvp(): """#388: Can't make iterators via make_iterator() with different r/v policies""" - import pybind11_tests.sequences_and_iterators as m - assert list(m.make_iterator_1()) == [1, 2, 3] assert list(m.make_iterator_2()) == [1, 2, 3] assert not isinstance(m.make_iterator_1(), type(m.make_iterator_2())) @@ -274,3 +272,25 @@ def test_carray_iterator(): arr_h = m.CArrayHolder(*args_gt) args = list(arr_h) assert args_gt == args + + +def test_generated_dunder_methods_pos_only(): + string_map = m.StringMap({"hi": "bye", "black": "white"}) + for it in ( + m.make_iterator_1(), + m.make_iterator_2(), + m.iterator_passthrough(iter([3, 5, 7])), + iter(m.Sequence(5)), + iter(string_map), + string_map.items(), + string_map.values(), + iter(m.CArrayHolder(*[float(i) for i in range(3)])), + ): + assert ( + re.match(r"^__iter__\(self: [\w\.]+, /\)", type(it).__iter__.__doc__) + is not None + ) + assert ( + re.match(r"^__next__\(self: [\w\.]+, /\)", type(it).__next__.__doc__) + is not None + ) From 0ed20f26acee626ac989568ecc6347e159ddbb47 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 11 Nov 2024 16:55:21 -0800 Subject: [PATCH 3/6] chore(deps): bump actions/attest-build-provenance in the actions group (#5440) Bumps the actions group with 1 update: [actions/attest-build-provenance](https://github.com/actions/attest-build-provenance). Updates `actions/attest-build-provenance` from 1.4.3 to 1.4.4 - [Release notes](https://github.com/actions/attest-build-provenance/releases) - [Changelog](https://github.com/actions/attest-build-provenance/blob/main/RELEASE.md) - [Commits](https://github.com/actions/attest-build-provenance/compare/1c608d11d69870c2092266b3f9a6f3abbf17002c...ef244123eb79f2f7a7e75d99086184180e6d0018) --- updated-dependencies: - dependency-name: actions/attest-build-provenance dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/pip.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pip.yml b/.github/workflows/pip.yml index 371353737..6d1799350 100644 --- a/.github/workflows/pip.yml +++ b/.github/workflows/pip.yml @@ -103,7 +103,7 @@ jobs: - uses: actions/download-artifact@v4 - name: Generate artifact attestation for sdist and wheel - uses: actions/attest-build-provenance@1c608d11d69870c2092266b3f9a6f3abbf17002c # v1.4.3 + uses: actions/attest-build-provenance@ef244123eb79f2f7a7e75d99086184180e6d0018 # v1.4.4 with: subject-path: "*/pybind11*" From 08095d9c702f8bcdfee385ea7f526b98cc9f358c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20=C5=A0im=C3=A1=C4=8Dek?= Date: Thu, 14 Nov 2024 18:03:56 +0100 Subject: [PATCH 4/6] Run pytest in verbose mode (#5443) --- .github/workflows/ci.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b8150a8a3..f47cfa35c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -145,6 +145,7 @@ jobs: -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON -DPYBIND11_SIMPLE_GIL_MANAGEMENT=ON -DPYBIND11_NUMPY_1_ONLY=ON + -DPYBIND11_PYTEST_ARGS=-v -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=11 @@ -174,6 +175,7 @@ jobs: -DPYBIND11_WERROR=ON -DPYBIND11_SIMPLE_GIL_MANAGEMENT=OFF -DPYBIND11_NUMPY_1_ONLY=ON + -DPYBIND11_PYTEST_ARGS=-v -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=17 @@ -193,6 +195,7 @@ jobs: run: > cmake -S . -B build3 -DPYBIND11_WERROR=ON + -DPYBIND11_PYTEST_ARGS=-v -DDOWNLOAD_CATCH=ON -DDOWNLOAD_EIGEN=ON -DCMAKE_CXX_STANDARD=17 From b9fb3168ab235deacc4434d9c67760b67a0862ce Mon Sep 17 00:00:00 2001 From: Maarten Baert Date: Sat, 16 Nov 2024 22:45:59 +0100 Subject: [PATCH 5/6] Add support for array_t and array_t (#5427) * Add support for array_t and array_t * style: pre-commit fixes * Remove loops that aren't strictly needed * Fix compiler warning * Disable GC-dependent checks when running on pypy or graalpy * style: pre-commit fixes * Remove PyValueHolder counter again * Move tests to templates to avoid code duplication * Rerun pre-commit * Restore import that was erroneously removed by pre-commit * Reduce code duplication with more template magic * Bring back `.attr("value")` in `return_array_cpp_loop()` This was meant to further stress-test correctness of refcount handling. All modified test functions were manually leak-checked (`while True`, top command, Python 3.12.3, Ubuntu 24.01, gcc 13.2.0). --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve --- include/pybind11/numpy.h | 6 ++- tests/test_numpy_array.cpp | 84 ++++++++++++++++++++++++++++---------- tests/test_numpy_array.py | 52 +++++++++++++++-------- 3 files changed, 102 insertions(+), 40 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 09894cf74..228e02c3d 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -1428,7 +1428,11 @@ public: }; template -struct npy_format_descriptor::value>> { +struct npy_format_descriptor< + T, + enable_if_t::value + || ((std::is_same::value || std::is_same::value) + && sizeof(T) == sizeof(PyObject *))>> { static constexpr auto name = const_name("object"); static constexpr int value = npy_api::NPY_OBJECT_; diff --git a/tests/test_numpy_array.cpp b/tests/test_numpy_array.cpp index c2f754208..79ade3ba1 100644 --- a/tests/test_numpy_array.cpp +++ b/tests/test_numpy_array.cpp @@ -156,6 +156,55 @@ py::handle auxiliaries(T &&r, T2 &&r2) { return l.release(); } +template +PyObjectType convert_to_pyobjecttype(py::object obj); + +template <> +PyObject *convert_to_pyobjecttype(py::object obj) { + return obj.release().ptr(); +} + +template <> +py::handle convert_to_pyobjecttype(py::object obj) { + return obj.release(); +} + +template <> +py::object convert_to_pyobjecttype(py::object obj) { + return obj; +} + +template +std::string pass_array_return_sum_str_values(const py::array_t &objs) { + std::string sum_str_values; + for (const auto &obj : objs) { + sum_str_values += py::str(obj.attr("value")); + } + return sum_str_values; +} + +template +py::list pass_array_return_as_list(const py::array_t &objs) { + return objs; +} + +template +py::array_t return_array_cpp_loop(const py::list &objs) { + py::size_t arr_size = py::len(objs); + py::array_t arr_from_list(static_cast(arr_size)); + PyObjectType *data = arr_from_list.mutable_data(); + for (py::size_t i = 0; i < arr_size; i++) { + assert(!data[i]); + data[i] = convert_to_pyobjecttype(objs[i].attr("value")); + } + return arr_from_list; +} + +template +py::array_t return_array_from_list(const py::list &objs) { + return objs; +} + // note: declaration at local scope would create a dangling reference! static int data_i = 42; @@ -520,28 +569,21 @@ TEST_SUBMODULE(numpy_array, sm) { sm.def("round_trip_float", [](double d) { return d; }); sm.def("pass_array_pyobject_ptr_return_sum_str_values", - [](const py::array_t &objs) { - std::string sum_str_values; - for (const auto &obj : objs) { - sum_str_values += py::str(obj.attr("value")); - } - return sum_str_values; - }); + pass_array_return_sum_str_values); + sm.def("pass_array_handle_return_sum_str_values", + pass_array_return_sum_str_values); + sm.def("pass_array_object_return_sum_str_values", + pass_array_return_sum_str_values); - sm.def("pass_array_pyobject_ptr_return_as_list", - [](const py::array_t &objs) -> py::list { return objs; }); + sm.def("pass_array_pyobject_ptr_return_as_list", pass_array_return_as_list); + sm.def("pass_array_handle_return_as_list", pass_array_return_as_list); + sm.def("pass_array_object_return_as_list", pass_array_return_as_list); - sm.def("return_array_pyobject_ptr_cpp_loop", [](const py::list &objs) { - py::size_t arr_size = py::len(objs); - py::array_t arr_from_list(static_cast(arr_size)); - PyObject **data = arr_from_list.mutable_data(); - for (py::size_t i = 0; i < arr_size; i++) { - assert(data[i] == nullptr); - data[i] = py::cast(objs[i].attr("value")); - } - return arr_from_list; - }); + sm.def("return_array_pyobject_ptr_cpp_loop", return_array_cpp_loop); + sm.def("return_array_handle_cpp_loop", return_array_cpp_loop); + sm.def("return_array_object_cpp_loop", return_array_cpp_loop); - sm.def("return_array_pyobject_ptr_from_list", - [](const py::list &objs) -> py::array_t { return objs; }); + sm.def("return_array_pyobject_ptr_from_list", return_array_from_list); + sm.def("return_array_handle_from_list", return_array_from_list); + sm.def("return_array_object_from_list", return_array_from_list); } diff --git a/tests/test_numpy_array.py b/tests/test_numpy_array.py index 6e8bde826..b1c6875f9 100644 --- a/tests/test_numpy_array.py +++ b/tests/test_numpy_array.py @@ -629,45 +629,61 @@ def UnwrapPyValueHolder(vhs): return [vh.value for vh in vhs] -def test_pass_array_pyobject_ptr_return_sum_str_values_ndarray(): +PASS_ARRAY_PYOBJECT_RETURN_SUM_STR_VALUES_FUNCTIONS = [ + m.pass_array_pyobject_ptr_return_sum_str_values, + m.pass_array_handle_return_sum_str_values, + m.pass_array_object_return_sum_str_values, +] + + +@pytest.mark.parametrize( + "pass_array", PASS_ARRAY_PYOBJECT_RETURN_SUM_STR_VALUES_FUNCTIONS +) +def test_pass_array_object_return_sum_str_values_ndarray(pass_array): # Intentionally all temporaries, do not change. assert ( - m.pass_array_pyobject_ptr_return_sum_str_values( - np.array(WrapWithPyValueHolder(-3, "four", 5.0), dtype=object) - ) + pass_array(np.array(WrapWithPyValueHolder(-3, "four", 5.0), dtype=object)) == "-3four5.0" ) -def test_pass_array_pyobject_ptr_return_sum_str_values_list(): +@pytest.mark.parametrize( + "pass_array", PASS_ARRAY_PYOBJECT_RETURN_SUM_STR_VALUES_FUNCTIONS +) +def test_pass_array_object_return_sum_str_values_list(pass_array): # Intentionally all temporaries, do not change. - assert ( - m.pass_array_pyobject_ptr_return_sum_str_values( - WrapWithPyValueHolder(2, "three", -4.0) - ) - == "2three-4.0" - ) + assert pass_array(WrapWithPyValueHolder(2, "three", -4.0)) == "2three-4.0" -def test_pass_array_pyobject_ptr_return_as_list(): +@pytest.mark.parametrize( + "pass_array", + [ + m.pass_array_pyobject_ptr_return_as_list, + m.pass_array_handle_return_as_list, + m.pass_array_object_return_as_list, + ], +) +def test_pass_array_object_return_as_list(pass_array): # Intentionally all temporaries, do not change. assert UnwrapPyValueHolder( - m.pass_array_pyobject_ptr_return_as_list( - np.array(WrapWithPyValueHolder(-1, "two", 3.0), dtype=object) - ) + pass_array(np.array(WrapWithPyValueHolder(-1, "two", 3.0), dtype=object)) ) == [-1, "two", 3.0] @pytest.mark.parametrize( - ("return_array_pyobject_ptr", "unwrap"), + ("return_array", "unwrap"), [ (m.return_array_pyobject_ptr_cpp_loop, list), + (m.return_array_handle_cpp_loop, list), + (m.return_array_object_cpp_loop, list), (m.return_array_pyobject_ptr_from_list, UnwrapPyValueHolder), + (m.return_array_handle_from_list, UnwrapPyValueHolder), + (m.return_array_object_from_list, UnwrapPyValueHolder), ], ) -def test_return_array_pyobject_ptr_cpp_loop(return_array_pyobject_ptr, unwrap): +def test_return_array_object_cpp_loop(return_array, unwrap): # Intentionally all temporaries, do not change. - arr_from_list = return_array_pyobject_ptr(WrapWithPyValueHolder(6, "seven", -8.0)) + arr_from_list = return_array(WrapWithPyValueHolder(6, "seven", -8.0)) assert isinstance(arr_from_list, np.ndarray) assert arr_from_list.dtype == np.dtype("O") assert unwrap(arr_from_list) == [6, "seven", -8.0] From f41dae31a3e2f810a7ca7f5380a8c8a8602a466a Mon Sep 17 00:00:00 2001 From: Maarten Baert Date: Sun, 17 Nov 2024 16:56:02 +0100 Subject: [PATCH 6/6] Add dtype::normalized_num and dtype::num_of (#5429) * Add dtype::normalized_num and dtype::num_of * Fix compiler warning and improve NumPy 1.x compatibility * Fix clang-tidy warning * Fix another clang-tidy warning * Add extra comment --- include/pybind11/numpy.h | 91 ++++++++++++++++++++++++++++++- tests/test_numpy_dtypes.cpp | 104 ++++++++++++++++++++++++++++++++++++ tests/test_numpy_dtypes.py | 22 ++++++++ 3 files changed, 216 insertions(+), 1 deletion(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 228e02c3d..ab224e1f1 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -212,6 +212,7 @@ constexpr int platform_lookup(int I, Ints... Is) { } struct npy_api { + // If you change this code, please review `normalized_dtype_num` below. enum constants { NPY_ARRAY_C_CONTIGUOUS_ = 0x0001, NPY_ARRAY_F_CONTIGUOUS_ = 0x0002, @@ -384,6 +385,74 @@ private: } }; +// This table normalizes typenums by mapping NPY_INT_, NPY_LONG, ... to NPY_INT32_, NPY_INT64, ... +// This is needed to correctly handle situations where multiple typenums map to the same type, +// e.g. NPY_LONG_ may be equivalent to NPY_INT_ or NPY_LONGLONG_ despite having a different +// typenum. The normalized typenum should always match the values used in npy_format_descriptor. +// If you change this code, please review `enum constants` above. +static constexpr int normalized_dtype_num[npy_api::NPY_VOID_ + 1] = { + // NPY_BOOL_ => + npy_api::NPY_BOOL_, + // NPY_BYTE_ => + npy_api::NPY_BYTE_, + // NPY_UBYTE_ => + npy_api::NPY_UBYTE_, + // NPY_SHORT_ => + npy_api::NPY_INT16_, + // NPY_USHORT_ => + npy_api::NPY_UINT16_, + // NPY_INT_ => + sizeof(int) == sizeof(std::int16_t) ? npy_api::NPY_INT16_ + : sizeof(int) == sizeof(std::int32_t) ? npy_api::NPY_INT32_ + : sizeof(int) == sizeof(std::int64_t) ? npy_api::NPY_INT64_ + : npy_api::NPY_INT_, + // NPY_UINT_ => + sizeof(unsigned int) == sizeof(std::uint16_t) ? npy_api::NPY_UINT16_ + : sizeof(unsigned int) == sizeof(std::uint32_t) ? npy_api::NPY_UINT32_ + : sizeof(unsigned int) == sizeof(std::uint64_t) ? npy_api::NPY_UINT64_ + : npy_api::NPY_UINT_, + // NPY_LONG_ => + sizeof(long) == sizeof(std::int16_t) ? npy_api::NPY_INT16_ + : sizeof(long) == sizeof(std::int32_t) ? npy_api::NPY_INT32_ + : sizeof(long) == sizeof(std::int64_t) ? npy_api::NPY_INT64_ + : npy_api::NPY_LONG_, + // NPY_ULONG_ => + sizeof(unsigned long) == sizeof(std::uint16_t) ? npy_api::NPY_UINT16_ + : sizeof(unsigned long) == sizeof(std::uint32_t) ? npy_api::NPY_UINT32_ + : sizeof(unsigned long) == sizeof(std::uint64_t) ? npy_api::NPY_UINT64_ + : npy_api::NPY_ULONG_, + // NPY_LONGLONG_ => + sizeof(long long) == sizeof(std::int16_t) ? npy_api::NPY_INT16_ + : sizeof(long long) == sizeof(std::int32_t) ? npy_api::NPY_INT32_ + : sizeof(long long) == sizeof(std::int64_t) ? npy_api::NPY_INT64_ + : npy_api::NPY_LONGLONG_, + // NPY_ULONGLONG_ => + sizeof(unsigned long long) == sizeof(std::uint16_t) ? npy_api::NPY_UINT16_ + : sizeof(unsigned long long) == sizeof(std::uint32_t) ? npy_api::NPY_UINT32_ + : sizeof(unsigned long long) == sizeof(std::uint64_t) ? npy_api::NPY_UINT64_ + : npy_api::NPY_ULONGLONG_, + // NPY_FLOAT_ => + npy_api::NPY_FLOAT_, + // NPY_DOUBLE_ => + npy_api::NPY_DOUBLE_, + // NPY_LONGDOUBLE_ => + npy_api::NPY_LONGDOUBLE_, + // NPY_CFLOAT_ => + npy_api::NPY_CFLOAT_, + // NPY_CDOUBLE_ => + npy_api::NPY_CDOUBLE_, + // NPY_CLONGDOUBLE_ => + npy_api::NPY_CLONGDOUBLE_, + // NPY_OBJECT_ => + npy_api::NPY_OBJECT_, + // NPY_STRING_ => + npy_api::NPY_STRING_, + // NPY_UNICODE_ => + npy_api::NPY_UNICODE_, + // NPY_VOID_ => + npy_api::NPY_VOID_, +}; + inline PyArray_Proxy *array_proxy(void *ptr) { return reinterpret_cast(ptr); } inline const PyArray_Proxy *array_proxy(const void *ptr) { @@ -684,6 +753,13 @@ public: return detail::npy_format_descriptor::type>::dtype(); } + /// Return the type number associated with a C++ type. + /// This is the constexpr equivalent of `dtype::of().num()`. + template + static constexpr int num_of() { + return detail::npy_format_descriptor::type>::value; + } + /// Size of the data type in bytes. #ifdef PYBIND11_NUMPY_1_ONLY ssize_t itemsize() const { return detail::array_descriptor_proxy(m_ptr)->elsize; } @@ -725,7 +801,9 @@ public: return detail::array_descriptor_proxy(m_ptr)->type; } - /// type number of dtype. + /// Type number of dtype. Note that different values may be returned for equivalent types, + /// e.g. even though ``long`` may be equivalent to ``int`` or ``long long``, they still have + /// different type numbers. Consider using `normalized_num` to avoid this. int num() const { // Note: The signature, `dtype::num` follows the naming of NumPy's public // Python API (i.e., ``dtype.num``), rather than its internal @@ -733,6 +811,17 @@ public: return detail::array_descriptor_proxy(m_ptr)->type_num; } + /// Type number of dtype, normalized to match the return value of `num_of` for equivalent + /// types. This function can be used to write switch statements that correctly handle + /// equivalent types with different type numbers. + int normalized_num() const { + int value = num(); + if (value >= 0 && value <= detail::npy_api::NPY_VOID_) { + return detail::normalized_dtype_num[value]; + } + return value; + } + /// Single character for byteorder char byteorder() const { return detail::array_descriptor_proxy(m_ptr)->byteorder; } diff --git a/tests/test_numpy_dtypes.cpp b/tests/test_numpy_dtypes.cpp index 596d90274..b6db439d9 100644 --- a/tests/test_numpy_dtypes.cpp +++ b/tests/test_numpy_dtypes.cpp @@ -11,6 +11,9 @@ #include "pybind11_tests.h" +#include +#include + #ifdef __GNUC__ # define PYBIND11_PACKED(cls) cls __attribute__((__packed__)) #else @@ -297,6 +300,15 @@ py::list test_dtype_ctors() { return list; } +template +py::array_t dispatch_array_increment(py::array_t arr) { + py::array_t res(arr.shape(0)); + for (py::ssize_t i = 0; i < arr.shape(0); ++i) { + res.mutable_at(i) = T(arr.at(i) + 1); + } + return res; +} + struct A {}; struct B {}; @@ -496,6 +508,98 @@ TEST_SUBMODULE(numpy_dtypes, m) { } return list; }); + m.def("test_dtype_num_of", []() -> py::list { + py::list res; +#define TEST_DTYPE(T) res.append(py::make_tuple(py::dtype::of().num(), py::dtype::num_of())); + TEST_DTYPE(bool) + TEST_DTYPE(char) + TEST_DTYPE(unsigned char) + TEST_DTYPE(short) + TEST_DTYPE(unsigned short) + TEST_DTYPE(int) + TEST_DTYPE(unsigned int) + TEST_DTYPE(long) + TEST_DTYPE(unsigned long) + TEST_DTYPE(long long) + TEST_DTYPE(unsigned long long) + TEST_DTYPE(float) + TEST_DTYPE(double) + TEST_DTYPE(long double) + TEST_DTYPE(std::complex) + TEST_DTYPE(std::complex) + TEST_DTYPE(std::complex) + TEST_DTYPE(int8_t) + TEST_DTYPE(uint8_t) + TEST_DTYPE(int16_t) + TEST_DTYPE(uint16_t) + TEST_DTYPE(int32_t) + TEST_DTYPE(uint32_t) + TEST_DTYPE(int64_t) + TEST_DTYPE(uint64_t) +#undef TEST_DTYPE + return res; + }); + m.def("test_dtype_normalized_num", []() -> py::list { + py::list res; +#define TEST_DTYPE(NT, T) \ + res.append(py::make_tuple(py::dtype(py::detail::npy_api::NT).normalized_num(), \ + py::dtype::num_of())); + TEST_DTYPE(NPY_BOOL_, bool) + TEST_DTYPE(NPY_BYTE_, char); + TEST_DTYPE(NPY_UBYTE_, unsigned char); + TEST_DTYPE(NPY_SHORT_, short); + TEST_DTYPE(NPY_USHORT_, unsigned short); + TEST_DTYPE(NPY_INT_, int); + TEST_DTYPE(NPY_UINT_, unsigned int); + TEST_DTYPE(NPY_LONG_, long); + TEST_DTYPE(NPY_ULONG_, unsigned long); + TEST_DTYPE(NPY_LONGLONG_, long long); + TEST_DTYPE(NPY_ULONGLONG_, unsigned long long); + TEST_DTYPE(NPY_FLOAT_, float); + TEST_DTYPE(NPY_DOUBLE_, double); + TEST_DTYPE(NPY_LONGDOUBLE_, long double); + TEST_DTYPE(NPY_CFLOAT_, std::complex); + TEST_DTYPE(NPY_CDOUBLE_, std::complex); + TEST_DTYPE(NPY_CLONGDOUBLE_, std::complex); + TEST_DTYPE(NPY_INT8_, int8_t); + TEST_DTYPE(NPY_UINT8_, uint8_t); + TEST_DTYPE(NPY_INT16_, int16_t); + TEST_DTYPE(NPY_UINT16_, uint16_t); + TEST_DTYPE(NPY_INT32_, int32_t); + TEST_DTYPE(NPY_UINT32_, uint32_t); + TEST_DTYPE(NPY_INT64_, int64_t); + TEST_DTYPE(NPY_UINT64_, uint64_t); +#undef TEST_DTYPE + return res; + }); + m.def("test_dtype_switch", [](const py::array &arr) -> py::array { + switch (arr.dtype().normalized_num()) { + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + case py::dtype::num_of(): + return dispatch_array_increment(arr); + default: + throw std::runtime_error("Unsupported dtype"); + } + }); m.def("test_dtype_methods", []() { py::list list; auto dt1 = py::dtype::of(); diff --git a/tests/test_numpy_dtypes.py b/tests/test_numpy_dtypes.py index 8ae239ed8..5d839933c 100644 --- a/tests/test_numpy_dtypes.py +++ b/tests/test_numpy_dtypes.py @@ -188,6 +188,28 @@ def test_dtype(simple_dtype): chr(np.dtype(ch).flags) for ch in expected_chars ] + for a, b in m.test_dtype_num_of(): + assert a == b + + for a, b in m.test_dtype_normalized_num(): + assert a == b + + arr = np.array([4, 84, 21, 36]) + # Note: "ulong" does not work in NumPy 1.x, so we use "L" + assert (m.test_dtype_switch(arr.astype("byte")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("ubyte")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("short")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("ushort")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("intc")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("uintc")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("long")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("L")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("longlong")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("ulonglong")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("single")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("double")) == arr + 1).all() + assert (m.test_dtype_switch(arr.astype("longdouble")) == arr + 1).all() + def test_recarray(simple_dtype, packed_dtype): elements = [(False, 0, 0.0, -0.0), (True, 1, 1.5, -2.5), (False, 2, 3.0, -5.0)]