diff --git a/Python/Core/CMakeLists.txt b/Python/Core/CMakeLists.txt index 15f369b7a3d..af0cff90de0 100644 --- a/Python/Core/CMakeLists.txt +++ b/Python/Core/CMakeLists.txt @@ -42,6 +42,16 @@ target_link_libraries( PUBLIC Acts::Core Acts::PythonUtilities ) +# Test-only module: a Core-only way to disown EventData containers (by taking +# them as std::unique_ptr) so the proxy fail-loud behaviour can be tested +# without acts.examples. Built into the python tree but never installed. +pybind11_add_module(_acts_core_test_bindings tests/DisownTestBindings.cpp) +target_link_libraries(_acts_core_test_bindings PRIVATE Acts::Core) +set_target_properties( + _acts_core_test_bindings + PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${_python_dir} +) + configure_file(setup.sh.in ${_python_dir}/setup.sh @ONLY) install(FILES ${_python_dir}/setup.sh DESTINATION python) diff --git a/Python/Core/src/EventData.cpp b/Python/Core/src/EventData.cpp index dad3a6fe1c2..b08a7df22ba 100644 --- a/Python/Core/src/EventData.cpp +++ b/Python/Core/src/EventData.cpp @@ -16,12 +16,16 @@ #include "Acts/EventData/Types.hpp" #include "Acts/Surfaces/CurvilinearSurface.hpp" #include "Acts/Surfaces/Surface.hpp" +#include "ActsPython/Utilities/ProxyTether.hpp" #include "ActsPython/Utilities/WhiteBoardRegistry.hpp" #include +#include #include #include #include +#include +#include #include #include @@ -85,6 +89,50 @@ auto arrayColumn(ArrayColumnGetter getColumn, }; } +/// Build the "missing column" error message for a space point accessor. +inline std::string missingColumnMessage(const std::string_view& columnName) { + return std::format( + "SpacePoint has no '{}' column; construct the container with " + "the corresponding SpacePointColumns flag", + columnName); +} + +/// Wrap a read accessor on a tethered space point proxy. First validates the +/// tether (raises py::value_error if the container was consumed/disowned), then +/// checks the column exists (raises py::attribute_error if missing), then calls +/// `access` with the inner proxy. `Tether` is given explicitly at the call +/// site; `access` is a callable taking `const Proxy&` (calling the accessor on +/// a const reference avoids the const/non-const overload ambiguity that would +/// otherwise force static_cast on plain member pointers). +template +auto guardedRead(SpacePointColumns requiredColumn, + const std::string_view& columnName, Access access) { + return [requiredColumn, columnName, access](const Tether& self) { + const auto& proxy = self.checked(); + if (!proxy.container().hasColumns(requiredColumn)) { + throw py::attribute_error(missingColumnMessage(columnName)); + } + return access(proxy); + }; +} + +/// Wrap a write accessor on a tethered mutable space point proxy: validate the +/// tether, then check the column exists, then assign. `V` is the concrete value +/// type and is given explicitly at the call site: the returned setter takes +/// `const V&` (never auto) so pybind11 can derive the Python->C++ argument +/// conversion. `assign` is a callable taking `Proxy&` and `const V&`. +template +auto guardedWrite(SpacePointColumns requiredColumn, + const std::string_view& columnName, Assign assign) { + return [requiredColumn, columnName, assign](Tether& self, const V& value) { + auto& proxy = self.checked(); + if (!proxy.container().hasColumns(requiredColumn)) { + throw py::attribute_error(missingColumnMessage(columnName)); + } + assign(proxy, value); + }; +} + /// @brief This adds the classes from Core/EventData to the python module /// @param m the pybind11 core module void addEventData(py::module_& m) { @@ -123,12 +171,33 @@ void addEventData(py::module_& m) { static_cast(b)); }); + // Proxies are bound as ProxyTether under the same Python names, so isinstance + // is preserved; see ProxyTether.hpp for the disown/keep-alive rationale. The + // aliases below keep the binding call sites readable. + using ConstSpTether = ProxyTether; + using MutSpTether = ProxyTether; + using ConstSeedTether = ProxyTether; + using MutSeedTether = ProxyTether; + constexpr auto spAlive = &ownerAlive; + constexpr auto seedAlive = &ownerAlive; + + // Register iterator types before the __iter__ bindings that return them. + bindCheckedIndexIterator( + m, "_SpacePointContainer2Iterator"); + bindCheckedIndexIterator(m, "_SeedContainer2Iterator"); + using FloatColumnGetter = ConstSpacePointColumnProxy (SpacePointContainer2::*)() const; - auto floatColumn = [](FloatColumnGetter column) { - return [column](const SpacePointContainer2& self) { - return spanToNumpy1d((self.*column)().data(), py::cast(self)); - }; + auto floatColumn = [](FloatColumnGetter column, + SpacePointColumns requiredColumn, + const std::string_view& columnName) { + return + [column, requiredColumn, columnName](const SpacePointContainer2& self) { + if (!self.hasColumns(requiredColumn)) { + throw py::attribute_error(missingColumnMessage(columnName)); + } + return spanToNumpy1d((self.*column)().data(), py::cast(self)); + }; }; // SpacePointContainer2 @@ -143,33 +212,61 @@ void addEventData(py::module_& m) { .def("reserve", &SpacePointContainer2::reserve, py::arg("size"), py::arg("averageSourceLinks") = 1) .def("clear", &SpacePointContainer2::clear) + .def("createSpacePoint", + [](py::object self) { + auto& c = self.cast(); + return MutSpTether{self, c.createSpacePoint(), spAlive}; + }) .def("__len__", &SpacePointContainer2::size) .def("__getitem__", - [](const SpacePointContainer2& self, SpacePointIndex2 idx) { - return ConstSpacePointProxy2(self, idx); + [](py::object self, SpacePointIndex2 idx) { + auto& c = self.cast(); + return MutSpTether{self, MutableSpacePointProxy2(c, idx), + spAlive}; }) .def("__iter__", - [](const SpacePointContainer2& self) { - return py::make_iterator(self.begin(), self.end()); + [](py::object self) { + return CheckedIndexIterator{ + self, 0, + [](const py::object& owner, SpacePointContainer2& c, + std::size_t i) { + return py::cast( + MutSpTether{owner, + MutableSpacePointProxy2( + c, static_cast(i)), + &ownerAlive}); + }}; }) .def_property_readonly("x", - floatColumn(&SpacePointContainer2::xColumn)) + floatColumn(&SpacePointContainer2::xColumn, + SpacePointColumns::X, "x")) .def_property_readonly("y", - floatColumn(&SpacePointContainer2::yColumn)) + floatColumn(&SpacePointContainer2::yColumn, + SpacePointColumns::Y, "y")) .def_property_readonly("z", - floatColumn(&SpacePointContainer2::zColumn)) + floatColumn(&SpacePointContainer2::zColumn, + SpacePointColumns::Z, "z")) .def_property_readonly("r", - floatColumn(&SpacePointContainer2::rColumn)) + floatColumn(&SpacePointContainer2::rColumn, + SpacePointColumns::R, "r")) .def_property_readonly("phi", - floatColumn(&SpacePointContainer2::phiColumn)) + floatColumn(&SpacePointContainer2::phiColumn, + SpacePointColumns::Phi, "phi")) .def_property_readonly("time", - floatColumn(&SpacePointContainer2::timeColumn)) + floatColumn(&SpacePointContainer2::timeColumn, + SpacePointColumns::Time, "time")) .def_property_readonly( - "varianceZ", floatColumn(&SpacePointContainer2::varianceZColumn)) + "varianceZ", + floatColumn(&SpacePointContainer2::varianceZColumn, + SpacePointColumns::VarianceZ, "varianceZ")) .def_property_readonly( - "varianceR", floatColumn(&SpacePointContainer2::varianceRColumn)) + "varianceR", + floatColumn(&SpacePointContainer2::varianceRColumn, + SpacePointColumns::VarianceR, "varianceR")) .def_property_readonly( - "varianceT", floatColumn(&SpacePointContainer2::varianceTColumn)) + "varianceT", + floatColumn(&SpacePointContainer2::varianceTColumn, + SpacePointColumns::VarianceT, "varianceT")) .def_property_readonly( "xyColumn", arrayColumn<2>(&SpacePointContainer2::xyColumn, SpacePointColumns::PackedXY, "xy")) @@ -201,41 +298,201 @@ void addEventData(py::module_& m) { WhiteBoardRegistry::registerClass(spc2); // ConstSpacePointProxy2 - // Note: SpacePointProxy2 has both mutable (requires(!ReadOnly)) and const - // overloads of x/y/z etc. with different return types. GCC 13 includes the - // constrained-out overloads in pointer-to-member resolution, so we must - // static_cast to the exact return type to disambiguate. - using FloatGetter = float (ConstSpacePointProxy2::*)() const noexcept; - py::class_(m, "ConstSpacePointProxy2") - .def_property_readonly("index", &ConstSpacePointProxy2::index) + // Note: every column is optional and only present when the corresponding + // SpacePointColumns flag was requested. The proxy accessors guard existence + // with assert only (no-op in release), so each getter is wrapped in + // guardedRead to raise py::attribute_error instead of dereferencing a missing + // column. The accessors are called on a const reference, which selects the + // const overload unambiguously (no static_cast needed). + py::class_(m, "ConstSpacePointProxy2") + .def_property_readonly("index", tetheredRead( + [](const ConstSpacePointProxy2& s) { + return s.index(); + })) .def_property_readonly( - "x", static_cast(&ConstSpacePointProxy2::x)) + "x", guardedRead( + SpacePointColumns::X, "x", + [](const ConstSpacePointProxy2& s) { return s.x(); })) .def_property_readonly( - "y", static_cast(&ConstSpacePointProxy2::y)) + "y", guardedRead( + SpacePointColumns::Y, "y", + [](const ConstSpacePointProxy2& s) { return s.y(); })) .def_property_readonly( - "z", static_cast(&ConstSpacePointProxy2::z)) + "z", guardedRead( + SpacePointColumns::Z, "z", + [](const ConstSpacePointProxy2& s) { return s.z(); })) .def_property_readonly( - "r", static_cast(&ConstSpacePointProxy2::r)) + "r", guardedRead( + SpacePointColumns::R, "r", + [](const ConstSpacePointProxy2& s) { return s.r(); })) .def_property_readonly( - "phi", static_cast(&ConstSpacePointProxy2::phi)) + "phi", guardedRead( + SpacePointColumns::Phi, "phi", + [](const ConstSpacePointProxy2& s) { return s.phi(); })) .def_property_readonly( - "time", static_cast(&ConstSpacePointProxy2::time)) + "time", guardedRead( + SpacePointColumns::Time, "time", + [](const ConstSpacePointProxy2& s) { return s.time(); })) .def_property_readonly( "varianceZ", - static_cast(&ConstSpacePointProxy2::varianceZ)) + guardedRead( + SpacePointColumns::VarianceZ, "varianceZ", + [](const ConstSpacePointProxy2& s) { return s.varianceZ(); })) .def_property_readonly( "varianceR", - static_cast(&ConstSpacePointProxy2::varianceR)) + guardedRead( + SpacePointColumns::VarianceR, "varianceR", + [](const ConstSpacePointProxy2& s) { return s.varianceR(); })) .def_property_readonly( "varianceT", - static_cast(&ConstSpacePointProxy2::varianceT)) + guardedRead( + SpacePointColumns::VarianceT, "varianceT", + [](const ConstSpacePointProxy2& s) { return s.varianceT(); })) .def_property_readonly( - "sourceLinks", py::cpp_function( - [](const ConstSpacePointProxy2& self) { - auto sls = self.sourceLinks(); - return py::make_iterator(sls.begin(), sls.end()); - }, - py::keep_alive<0, 1>())); + "sourceLinks", + py::cpp_function(guardedRead( + SpacePointColumns::SourceLinks, "sourceLinks", + [](const ConstSpacePointProxy2& self) { + auto sls = self.sourceLinks(); + return py::make_iterator(sls.begin(), + sls.end()); + }), + py::keep_alive<0, 1>())); + + // MutableSpacePointProxy2 + // Getters and setters are wrapped in guardedRead/guardedWrite so accessing or + // writing a column that was not requested at construction raises + // py::attribute_error instead of dereferencing a missing (disengaged) + // optional column. + using MutProxy = MutableSpacePointProxy2; + py::class_(m, "MutableSpacePointProxy2") + .def_property_readonly("index", + tetheredRead( + [](const MutProxy& s) { return s.index(); })) + .def_property( + "x", + guardedRead(SpacePointColumns::X, "x", + [](const MutProxy& s) { return s.x(); }), + guardedWrite( + SpacePointColumns::X, "x", + [](MutProxy& s, const float& v) { s.x() = v; })) + .def_property( + "y", + guardedRead(SpacePointColumns::Y, "y", + [](const MutProxy& s) { return s.y(); }), + guardedWrite( + SpacePointColumns::Y, "y", + [](MutProxy& s, const float& v) { s.y() = v; })) + .def_property( + "z", + guardedRead(SpacePointColumns::Z, "z", + [](const MutProxy& s) { return s.z(); }), + guardedWrite( + SpacePointColumns::Z, "z", + [](MutProxy& s, const float& v) { s.z() = v; })) + .def_property( + "r", + guardedRead(SpacePointColumns::R, "r", + [](const MutProxy& s) { return s.r(); }), + guardedWrite( + SpacePointColumns::R, "r", + [](MutProxy& s, const float& v) { s.r() = v; })) + .def_property( + "phi", + guardedRead(SpacePointColumns::Phi, "phi", + [](const MutProxy& s) { return s.phi(); }), + guardedWrite( + SpacePointColumns::Phi, "phi", + [](MutProxy& s, const float& v) { s.phi() = v; })) + .def_property( + "time", + guardedRead(SpacePointColumns::Time, "time", + [](const MutProxy& s) { return s.time(); }), + guardedWrite( + SpacePointColumns::Time, "time", + [](MutProxy& s, const float& v) { s.time() = v; })) + .def_property("varianceZ", + guardedRead( + SpacePointColumns::VarianceZ, "varianceZ", + [](const MutProxy& s) { return s.varianceZ(); }), + guardedWrite( + SpacePointColumns::VarianceZ, "varianceZ", + [](MutProxy& s, const float& v) { s.varianceZ() = v; })) + .def_property("varianceR", + guardedRead( + SpacePointColumns::VarianceR, "varianceR", + [](const MutProxy& s) { return s.varianceR(); }), + guardedWrite( + SpacePointColumns::VarianceR, "varianceR", + [](MutProxy& s, const float& v) { s.varianceR() = v; })) + .def_property("varianceT", + guardedRead( + SpacePointColumns::VarianceT, "varianceT", + [](const MutProxy& s) { return s.varianceT(); }), + guardedWrite( + SpacePointColumns::VarianceT, "varianceT", + [](MutProxy& s, const float& v) { s.varianceT() = v; })) + .def_property("topStripVector", + guardedRead( + SpacePointColumns::TopStripVector, "topStripVector", + [](const MutProxy& s) { return s.topStripVector(); }), + guardedWrite>( + SpacePointColumns::TopStripVector, "topStripVector", + [](MutProxy& s, const std::array& v) { + s.topStripVector() = v; + })) + .def_property( + "bottomStripVector", + guardedRead( + SpacePointColumns::BottomStripVector, "bottomStripVector", + [](const MutProxy& s) { return s.bottomStripVector(); }), + guardedWrite>( + SpacePointColumns::BottomStripVector, "bottomStripVector", + [](MutProxy& s, const std::array& v) { + s.bottomStripVector() = v; + })) + .def_property( + "stripCenterDistance", + guardedRead( + SpacePointColumns::StripCenterDistance, "stripCenterDistance", + [](const MutProxy& s) { return s.stripCenterDistance(); }), + guardedWrite>( + SpacePointColumns::StripCenterDistance, "stripCenterDistance", + [](MutProxy& s, const std::array& v) { + s.stripCenterDistance() = v; + })) + .def_property("topStripCenter", + guardedRead( + SpacePointColumns::TopStripCenter, "topStripCenter", + [](const MutProxy& s) { return s.topStripCenter(); }), + guardedWrite>( + SpacePointColumns::TopStripCenter, "topStripCenter", + [](MutProxy& s, const std::array& v) { + s.topStripCenter() = v; + })) + .def_property("copyFromIndex", + guardedRead( + SpacePointColumns::CopyFromIndex, "copyFromIndex", + [](const MutProxy& s) { return s.copyFromIndex(); }), + guardedWrite( + SpacePointColumns::CopyFromIndex, "copyFromIndex", + [](MutProxy& s, const SpacePointIndex2& v) { + s.copyFromIndex() = v; + })) + .def("assignSourceLinks", + [](MutSpTether& self, const std::vector& sourceLinks) { + self.checked().assignSourceLinks(sourceLinks); + }) + .def_property_readonly( + "sourceLinks", + py::cpp_function(guardedRead( + SpacePointColumns::SourceLinks, "sourceLinks", + [](const MutProxy& self) { + auto sls = self.sourceLinks(); + return py::make_iterator(sls.begin(), + sls.end()); + }), + py::keep_alive<0, 1>())); // SeedContainer2 auto seedContainer2 = @@ -245,27 +502,92 @@ void addEventData(py::module_& m) { .def_property_readonly("empty", &SeedContainer2::empty) .def("__len__", &SeedContainer2::size) .def("__getitem__", - [](const SeedContainer2& self, SeedIndex2 idx) { - return ConstSeedProxy2(self, idx); + [](py::object self, SeedIndex2 idx) { + const auto& c = self.cast(); + return ConstSeedTether{self, ConstSeedProxy2(c, idx), + seedAlive}; }) - .def("__iter__", [](const SeedContainer2& self) { - return py::make_iterator(self.begin(), self.end()); - }); + .def( + "__iter__", + [](py::object self) { + return CheckedIndexIterator{ + self, 0, + [](const py::object& owner, SeedContainer2& c, + std::size_t i) { + return py::cast(ConstSeedTether{ + owner, ConstSeedProxy2(c, static_cast(i)), + &ownerAlive}); + }}; + }) + .def("createSeed", + [](py::object self) { + auto& c = self.cast(); + return MutSeedTether{self, c.createSeed(), seedAlive}; + }) + .def( + "assignSpacePointContainer", + [](SeedContainer2& self, const SpacePointContainer2& sp) { + self.assignSpacePointContainer(sp); + }, + py::keep_alive<1, 2>()); WhiteBoardRegistry::registerClass(seedContainer2); - // ConstSeedProxy2 - py::class_(m, "ConstSeedProxy2") - .def_property_readonly("index", &ConstSeedProxy2::index) - .def_property_readonly("size", &ConstSeedProxy2::size) - .def_property_readonly("empty", &ConstSeedProxy2::empty) - .def_property_readonly("quality", &ConstSeedProxy2::quality) - .def_property_readonly("vertexZ", &ConstSeedProxy2::vertexZ) + // ConstSeedProxy2 (bound as a tether; seeds have no optional columns, so the + // accessors only need the disown check). + py::class_(m, "ConstSeedProxy2") + .def_property_readonly( + "index", tetheredRead( + [](const ConstSeedProxy2& s) { return s.index(); })) + .def_property_readonly( + "size", tetheredRead( + [](const ConstSeedProxy2& s) { return s.size(); })) + .def_property_readonly( + "empty", tetheredRead( + [](const ConstSeedProxy2& s) { return s.empty(); })) .def_property_readonly( - "spacePointIndices", [](const ConstSeedProxy2& self) { - return spanToNumpy1d(self.spacePointIndices(), py::cast(self)); + "quality", tetheredRead( + [](const ConstSeedProxy2& s) { return s.quality(); })) + .def_property_readonly( + "vertexZ", tetheredRead( + [](const ConstSeedProxy2& s) { return s.vertexZ(); })) + // spacePointIndices needs the owner (container) py::object as the numpy + // base, so it is a custom tethered accessor rather than tetheredRead. + .def_property_readonly( + "spacePointIndices", [](const ConstSeedTether& self) { + const auto& proxy = self.checked(); + return spanToNumpy1d(proxy.spacePointIndices(), self.owner); }); + // MutableSeedProxy2 (tether; disown check only). + py::class_(m, "MutableSeedProxy2") + .def_property_readonly( + "index", tetheredRead( + [](const MutableSeedProxy2& s) { return s.index(); })) + .def_property_readonly( + "size", tetheredRead( + [](const MutableSeedProxy2& s) { return s.size(); })) + .def_property_readonly( + "empty", tetheredRead( + [](const MutableSeedProxy2& s) { return s.empty(); })) + .def_property( + "quality", + tetheredRead( + [](const MutableSeedProxy2& s) { return s.quality(); }), + tetheredWrite( + [](MutableSeedProxy2& s, const float& q) { s.quality() = q; })) + .def_property( + "vertexZ", + tetheredRead( + [](const MutableSeedProxy2& s) { return s.vertexZ(); }), + tetheredWrite( + [](MutableSeedProxy2& s, const float& z) { s.vertexZ() = z; })) + .def("assignSpacePointIndices", + [](MutSeedTether& self, + const std::vector& indices) { + self.checked().assignSpacePointIndices(indices); + }); + // BoundTrackParameters (alias for // GenericBoundTrackParameters) py::class_(m, "BoundTrackParameters") diff --git a/Python/Core/tests/DisownTestBindings.cpp b/Python/Core/tests/DisownTestBindings.cpp new file mode 100644 index 00000000000..3cd73703ada --- /dev/null +++ b/Python/Core/tests/DisownTestBindings.cpp @@ -0,0 +1,35 @@ +// This file is part of the ACTS project. +// +// Copyright (C) 2016 CERN for the benefit of the ACTS project +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// Test-only pybind11 module (NOT part of the production `acts` package). It +// provides a Core-only way to disown a smart_holder container the same way the +// whiteboard does (by taking it as a std::unique_ptr), so the fail-loud +// behaviour of the EventData proxy tethers can be tested without depending on +// acts.examples. pybind11's type registry is process-wide, so these functions +// resolve the SpacePointContainer2 / SeedContainer2 types registered by +// `import acts`. + +#include "Acts/EventData/SeedContainer2.hpp" +#include "Acts/EventData/SpacePointContainer2.hpp" + +#include + +#include + +namespace py = pybind11; + +PYBIND11_MODULE(_acts_core_test_bindings, m) { + m.doc() = "Test-only helpers to disown ACTS EventData containers"; + + // Taking the argument by std::unique_ptr tells pybind11's smart_holder to + // disown the Python wrapper (and frees the object when the unique_ptr goes + // out of scope) -- exactly what WhiteBoardRegistry::fromPython does. + m.def("consume_spacepoints", + [](std::unique_ptr) {}); + m.def("consume_seeds", [](std::unique_ptr) {}); +} diff --git a/Python/Core/tests/test_event_data.py b/Python/Core/tests/test_event_data.py index effeb90627b..16d46a5b4dc 100644 --- a/Python/Core/tests/test_event_data.py +++ b/Python/Core/tests/test_event_data.py @@ -1,29 +1,280 @@ +import gc +import pytest import acts def test_space_point_container(): - """Test SpacePointContainer2 and ConstSpacePointProxy2 bindings (read-only).""" + """Test SpacePointContainer2 and MutableSpacePointProxy2 bindings.""" columns = ( acts.SpacePointColumns.X | acts.SpacePointColumns.Y | acts.SpacePointColumns.Z ) container = acts.SpacePointContainer2(columns) assert container.empty assert len(container) == 0 - - # Mutable bindings removed - container is read-only from Python - # Can still test iteration and len on empty container assert list(container) == [] + # createSpacePoint returns a MutableSpacePointProxy2 + sp0 = container.createSpacePoint() + assert isinstance(sp0, acts.MutableSpacePointProxy2) + assert not container.empty + assert len(container) == 1 + assert sp0.index == 0 + + # Write and read back scalar fields + sp0.x = 1.0 + sp0.y = 2.0 + sp0.z = 3.0 + assert sp0.x == pytest.approx(1.0) + assert sp0.y == pytest.approx(2.0) + assert sp0.z == pytest.approx(3.0) + + # __getitem__ returns MutableSpacePointProxy2 + via_index = container[0] + assert isinstance(via_index, acts.MutableSpacePointProxy2) + assert via_index.x == pytest.approx(1.0) + + # Mutation via __getitem__ proxy is reflected in the container + via_index.x = 99.0 + assert sp0.x == pytest.approx(99.0) + + # __iter__ yields MutableSpacePointProxy2 + sp1 = container.createSpacePoint() + sp1.x = 5.0 + items = list(container) + assert len(items) == 2 + assert isinstance(items[0], acts.MutableSpacePointProxy2) + assert items[0].x == pytest.approx(99.0) + assert items[1].x == pytest.approx(5.0) + + +def test_space_point_all_columns_round_trip(): + """Read/write every guarded column when the columns are present.""" + Cols = acts.SpacePointColumns + columns = ( + Cols.X + | Cols.Y + | Cols.Z + | Cols.R + | Cols.Phi + | Cols.Time + | Cols.VarianceZ + | Cols.VarianceR + | Cols.VarianceT + | Cols.TopStripVector + | Cols.BottomStripVector + | Cols.StripCenterDistance + | Cols.TopStripCenter + | Cols.CopyFromIndex + ) + container = acts.SpacePointContainer2(columns) + sp = container.createSpacePoint() + + # Scalar fields, including varianceT (writeable in C++ but previously not + # exposed on the mutable proxy). + sp.x = 1.0 + sp.y = 2.0 + sp.z = 3.0 + sp.r = 4.0 + sp.phi = 0.5 + sp.time = 6.0 + sp.varianceZ = 7.0 + sp.varianceR = 8.0 + sp.varianceT = 9.0 + assert sp.x == pytest.approx(1.0) + assert sp.time == pytest.approx(6.0) + assert sp.varianceT == pytest.approx(9.0) + + # Array (strip) fields. + sp.topStripVector = [1.0, 0.0, 0.0] + sp.bottomStripVector = [0.0, 1.0, 0.0] + sp.stripCenterDistance = [0.0, 0.0, 1.0] + sp.topStripCenter = [1.0, 1.0, 1.0] + assert list(sp.topStripVector) == pytest.approx([1.0, 0.0, 0.0]) + assert list(sp.topStripCenter) == pytest.approx([1.0, 1.0, 1.0]) + + sp.copyFromIndex = 0 + assert sp.copyFromIndex == 0 + + # Container-level numpy column views work when the column is present. + assert container.x.shape == (1,) + assert container.varianceT.shape == (1,) + + # Const proxy reads the same values. + const_sp = container[0] + assert const_sp.varianceT == pytest.approx(9.0) + + +def test_space_point_missing_column_raises(): + """Accessing a column that was not requested raises AttributeError + instead of segfaulting on a disengaged optional column.""" + Cols = acts.SpacePointColumns + + # Proxy read of a column absent from an otherwise-populated container. + container = acts.SpacePointContainer2(Cols.X) + sp = container.createSpacePoint() + with pytest.raises(AttributeError, match="time"): + _ = sp.time + + # Proxy read on a fully empty (None) container: even x is absent. + none_container = acts.SpacePointContainer2() + none_sp = none_container.createSpacePoint() + with pytest.raises(AttributeError, match="x"): + _ = none_sp.x + + # Proxy write to an absent scalar column. + with pytest.raises(AttributeError, match="time"): + sp.time = 1.0 + + # Proxy write to an absent array column. + with pytest.raises(AttributeError, match="topStripVector"): + sp.topStripVector = [0.0, 0.0, 0.0] + + # sourceLinks read without the SourceLinks column. + with pytest.raises(AttributeError, match="sourceLinks"): + _ = sp.sourceLinks + + # Const proxy read of an absent column. + const_sp = container[0] + with pytest.raises(AttributeError, match="varianceZ"): + _ = const_sp.varianceZ + + # Container-level numpy column property of an absent column. + with pytest.raises(AttributeError, match="time"): + _ = container.time + def test_seed_container(): - """Test SeedContainer2 and ConstSeedProxy2 bindings (read-only).""" + """Test SeedContainer2, MutableSeedProxy2 and ConstSeedProxy2 bindings.""" seed_container = acts.SeedContainer2() assert seed_container.empty assert len(seed_container) == 0 - - # Mutable bindings removed - container is read-only from Python assert list(seed_container) == [] + # createSeed returns a MutableSeedProxy2 + seed0 = seed_container.createSeed() + assert isinstance(seed0, acts.MutableSeedProxy2) + assert not seed_container.empty + assert len(seed_container) == 1 + + # Newly created seed is empty and has default values + assert seed0.empty + assert seed0.index == 0 + assert seed0.size == 0 + + # Write quality and vertexZ + seed0.quality = 0.9 + seed0.vertexZ = 12.5 + assert seed0.quality == pytest.approx(0.9) + assert seed0.vertexZ == pytest.approx(12.5) + + # Assign space point indices and verify size + seed0.assignSpacePointIndices([0, 1, 2]) + assert not seed0.empty + assert seed0.size == 3 + + # Create a second seed + seed1 = seed_container.createSeed() + assert seed1.index == 1 + assert len(seed_container) == 2 + + # Read back via ConstSeedProxy2 (__getitem__) + const0 = seed_container[0] + assert isinstance(const0, acts.ConstSeedProxy2) + assert const0.quality == pytest.approx(0.9) + assert const0.vertexZ == pytest.approx(12.5) + assert const0.size == 3 + assert list(const0.spacePointIndices) == [0, 1, 2] + + # Iteration yields ConstSeedProxy2 objects + seeds = list(seed_container) + assert len(seeds) == 2 + assert isinstance(seeds[0], acts.ConstSeedProxy2) + + # assignSpacePointContainer links the two containers (shared ownership). + # After the call, spacePoints() on a seed proxy is resolvable. + sp_container = acts.SpacePointContainer2( + acts.SpacePointColumns.X | acts.SpacePointColumns.Y | acts.SpacePointColumns.Z + ) + seed_container.assignSpacePointContainer(sp_container) + del sp_container + gc.collect() + assert list(const0.spacePointIndices) == [0, 1, 2] + + +def test_space_point_proxy_fails_loud_after_disown(): + """A space point proxy/iterator whose container was transferred away (the + whiteboard takes it as a unique_ptr, disowning the Python wrapper) must raise + ValueError on access instead of dereferencing freed memory (SIGSEGV).""" + tb = pytest.importorskip("_acts_core_test_bindings") + + Cols = acts.SpacePointColumns + container = acts.SpacePointContainer2(Cols.X | Cols.Y | Cols.Z) + sp = container.createSpacePoint() + sp.x = 1.0 + via_index = container[0] + it = iter(container) + + # Positive control: everything works while the container is owned. + assert sp.x == pytest.approx(1.0) + assert via_index.x == pytest.approx(1.0) + + # Transfer the container away (same mechanism as a whiteboard write). + tb.consume_spacepoints(container) + + # Every previously-obtained handle now fails loud rather than segfaulting. + with pytest.raises(ValueError, match="consumed"): + _ = sp.x + with pytest.raises(ValueError, match="consumed"): + _ = via_index.x + with pytest.raises(ValueError, match="consumed"): + next(it) + + +def test_seed_proxy_fails_loud_after_disown(): + """Same fail-loud contract for seed proxies/iterators after the container is + disowned by a unique_ptr transfer.""" + tb = pytest.importorskip("_acts_core_test_bindings") + + container = acts.SeedContainer2() + seed = container.createSeed() + seed.quality = 0.9 + via_index = container[0] + it = iter(container) + + assert seed.quality == pytest.approx(0.9) + assert via_index.quality == pytest.approx(0.9) + + tb.consume_seeds(container) + + with pytest.raises(ValueError, match="consumed"): + _ = seed.quality + with pytest.raises(ValueError, match="consumed"): + _ = via_index.quality + with pytest.raises(ValueError, match="consumed"): + next(it) + + +def test_proxy_survives_container_gc(): + """The tether holds a strong reference to its container, so a proxy keeps + working after the local container handle is dropped and collected (the + container is only truly gone once transferred, not merely unreferenced).""" + Cols = acts.SpacePointColumns + sp_container = acts.SpacePointContainer2(Cols.X) + sp = sp_container.createSpacePoint() + sp.x = 7.0 + del sp_container + gc.collect() + assert sp.x == pytest.approx(7.0) + assert isinstance(sp, acts.MutableSpacePointProxy2) + + seed_container = acts.SeedContainer2() + seed = seed_container.createSeed() + seed.quality = 3.0 + del seed_container + gc.collect() + assert seed.quality == pytest.approx(3.0) + assert isinstance(seed, acts.MutableSeedProxy2) + def test_particle_hypothesis(): muon = acts.ParticleHypothesis.muon diff --git a/Python/Examples/src/EventData.cpp b/Python/Examples/src/EventData.cpp index b89ff6880b9..0b72a7577ab 100644 --- a/Python/Examples/src/EventData.cpp +++ b/Python/Examples/src/EventData.cpp @@ -13,8 +13,11 @@ #include "ActsExamples/EventData/ProtoTrack.hpp" #include "ActsExamples/EventData/Track.hpp" #include "ActsExamples/EventData/TruthMatching.hpp" +#include "ActsPython/Utilities/ProxyTether.hpp" #include "ActsPython/Utilities/WhiteBoardRegistry.hpp" +#include + #include #include #include @@ -40,15 +43,19 @@ namespace { template auto bindFlatMultimap(py::module& m, const char* name) { + // Register the iterator type before the __iter__ binding that returns it. + const std::string iterName = std::string(name) + "Iterator"; + ActsPython::bindCheckedIterator(m, iterName.c_str()); + auto cls = py::classh(m, name) .def(py::init<>()) .def("__len__", &Map::size) - .def( - "__iter__", - [](const Map& self) { - return py::make_iterator(self.begin(), self.end()); - }, - py::keep_alive<0, 1>()) + .def("__iter__", + [](py::object self) { + const auto& map = self.cast(); + return ActsPython::CheckedIterator{ + self, py::make_iterator(map.begin(), map.end())}; + }) .def("__contains__", [](const Map& self, const typename Map::key_type& key) { return self.find(key) != self.end(); @@ -330,12 +337,16 @@ void addEventData(py::module& mex) { auto constTrackContainer = py::classh(mex, "ConstTrackContainer") .def("__len__", &ConstTrackContainer::size) - .def("__iter__", - [](const ConstTrackContainer& self) { - return py::make_iterator(self.begin(), self.end()); - }) - .def("__getitem__", py::overload_cast( - &ConstTrackContainer::getTrack, py::const_)) + .def( + "__iter__", + [](const ConstTrackContainer& self) { + return py::make_iterator(self.begin(), self.end()); + }, + py::keep_alive<0, 1>()) + .def("__getitem__", + py::overload_cast( + &ConstTrackContainer::getTrack, py::const_), + py::keep_alive<0, 1>()) // Zero-copy numpy array views of the underlying SoA columns. // The returned arrays are read-only and keep the container alive via @@ -487,23 +498,44 @@ void addEventData(py::module& mex) { }); // bind measurements - py::class_( - mex, "ConstVariableBoundMeasurementProxy") - .def_property_readonly("geometryId", - &ConstVariableBoundMeasurementProxy::geometryId) - .def_property_readonly("size", &ConstVariableBoundMeasurementProxy::size) - .def_property_readonly("index", - &ConstVariableBoundMeasurementProxy::index) + // The measurement proxy is bound as a ProxyTether (see ProxyTether.hpp). The + // type-erased alive-check lets both MeasurementContainer and + // MeasurementSubset produce the same bound proxy type. + using MeasTether = ProxyTether; + constexpr auto mcAlive = &ownerAlive; + constexpr auto msAlive = &ownerAlive; + + // Register iterator types before the __iter__ bindings that return them. + bindCheckedIndexIterator( + mex, "_MeasurementContainerIterator"); + bindCheckedIndexIterator(mex, + "_MeasurementSubsetIterator"); + + using MeasProxy = ConstVariableBoundMeasurementProxy; + py::class_(mex, "ConstVariableBoundMeasurementProxy") .def_property_readonly( - "fullParameters", &ConstVariableBoundMeasurementProxy::fullParameters) + "geometryId", tetheredRead( + [](const MeasProxy& s) { return s.geometryId(); })) .def_property_readonly( - "fullCovariance", &ConstVariableBoundMeasurementProxy::fullCovariance) + "size", + tetheredRead([](const MeasProxy& s) { return s.size(); })) + .def_property_readonly("index", + tetheredRead( + [](const MeasProxy& s) { return s.index(); })) + .def_property_readonly("fullParameters", + tetheredRead([](const MeasProxy& s) { + return s.fullParameters(); + })) + .def_property_readonly("fullCovariance", + tetheredRead([](const MeasProxy& s) { + return s.fullCovariance(); + })) .def_property_readonly( "subspaceIndices", - [](const ConstVariableBoundMeasurementProxy& self) { + tetheredRead([](const MeasProxy& self) { auto indices = self.subspaceHelper().indices(); return std::vector(indices.begin(), indices.end()); - }); + })); auto measurementContainer = py::classh(mex, "MeasurementContainer") @@ -512,11 +544,11 @@ void addEventData(py::module& mex) { .def("reserve", &MeasurementContainer::reserve) .def( "emplaceMeasurement", - [](MeasurementContainer& self, - Acts::GeometryIdentifier geometryId, + [](py::object self, Acts::GeometryIdentifier geometryId, const std::vector& indices, - const std::vector& par, const std::vector& cov) - -> ConstVariableBoundMeasurementProxy { + const std::vector& par, + const std::vector& cov) -> MeasTether { + auto& container = self.cast(); if (indices.size() != par.size() || indices.size() != cov.size()) { throw std::invalid_argument( @@ -537,22 +569,32 @@ void addEventData(py::module& mex) { dParams.indices = boundIndices; dParams.values = par; dParams.variances = cov; - return ConstVariableBoundMeasurementProxy{ - createMeasurement(self, geometryId, dParams)}; + return MeasTether{ + self, + ConstVariableBoundMeasurementProxy{ + createMeasurement(container, geometryId, dParams)}, + mcAlive}; }, py::arg("geometryId"), py::arg("indices"), py::arg("parameters"), py::arg("covariance")) .def("__getitem__", - [](const MeasurementContainer& self, - MeasurementContainer::Index idx) { - return self.getMeasurement(idx); + [](py::object self, MeasurementContainer::Index idx) { + const auto& container = + self.cast(); + return MeasTether{self, container.getMeasurement(idx), + mcAlive}; }) - .def( - "__iter__", - [](const MeasurementContainer& self) { - return py::make_iterator(self.begin(), self.end()); - }, - py::keep_alive<0, 1>()); + .def("__iter__", [](py::object self) { + return CheckedIndexIterator{ + self, 0, + [](const py::object& owner, MeasurementContainer& c, + std::size_t i) { + const MeasurementContainer& cc = c; + return py::cast( + MeasTether{owner, cc.getMeasurement(i), + &ownerAlive}); + }}; + }); WhiteBoardRegistry::registerClass(measurementContainer); @@ -568,23 +610,29 @@ void addEventData(py::module& mex) { .def("__len__", [](const MeasurementSubset& self) { return self.size(); }) .def("__getitem__", - [](const MeasurementSubset& self, std::size_t i) { - if (i >= self.size()) { + [](py::object self, std::size_t i) { + const auto& subset = self.cast(); + if (i >= subset.size()) { throw py::index_error("index out of range"); } - return self.at(i); + return MeasTether{self, subset.at(i), msAlive}; + }) + .def("__iter__", + [](py::object self) { + return CheckedIndexIterator{ + self, 0, + [](const py::object& owner, MeasurementSubset& c, + std::size_t i) { + const MeasurementSubset& cc = c; + return py::cast(MeasTether{ + owner, cc.at(i), &ownerAlive}); + }}; }) - .def( - "__iter__", - [](const MeasurementSubset& self) { - return py::make_iterator(self.begin(), self.end()); - }, - py::keep_alive<0, 1>()) .def( "getMeasurement", - [](const MeasurementSubset& self, - MeasurementContainer::Index idx) { - return self.getMeasurement(idx); + [](py::object self, MeasurementContainer::Index idx) { + const auto& subset = self.cast(); + return MeasTether{self, subset.getMeasurement(idx), msAlive}; }, py::arg("index")); diff --git a/Python/Utilities/include/ActsPython/Utilities/ProxyTether.hpp b/Python/Utilities/include/ActsPython/Utilities/ProxyTether.hpp new file mode 100644 index 00000000000..b1054a33e52 --- /dev/null +++ b/Python/Utilities/include/ActsPython/Utilities/ProxyTether.hpp @@ -0,0 +1,168 @@ +// This file is part of the ACTS project. +// +// Copyright (C) 2016 CERN for the benefit of the ACTS project +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +#pragma once + +#include +#include + +#include + +namespace ActsPython { + +/// Several EventData proxies/iterators hold a raw `Container*` + index and are +/// handed to Python. When the backing container is transferred to the +/// whiteboard, pybind11's smart_holder disowns the Python wrapper and the C++ +/// object is moved out and freed, leaving the proxy dangling (a later access is +/// a use-after-free -> SIGSEGV). +/// +/// `keep_alive` cannot help: it tracks the Python object's lifetime, but after +/// the transfer the wrapper is a live-but-disowned shell. The data genuinely +/// leaves, so the correct behaviour is to fail loud. pybind11 already maintains +/// the authoritative "is it still owned" signal (set when the transfer does +/// `py::cast>`), and we consult it by tethering each +/// proxy/iterator to its parent container's `py::object` and re-validating on +/// access. Holding the `py::object` also keeps the container alive while the +/// proxy lives, which subsumes `keep_alive` for the garbage-collection path. + +/// Return whether `owner`'s wrapped C++ container is still owned (not disowned +/// by a whiteboard transfer). Implemented by attempting to re-acquire the C++ +/// reference: this throws iff the instance was disowned. Any pending Python +/// error from the failed cast is cleared so the caller can raise a clean one. +template +bool ownerAlive(const pybind11::object& owner) { + try { + owner.cast(); + return true; + } catch (...) { + PyErr_Clear(); + return false; + } +} + +/// Tethers a C++ proxy to its parent container's Python object. The disown +/// check is type-erased via a function pointer (`&ownerAlive`) +/// rather than a Container template parameter, so a single bound proxy type can +/// be produced by several different container types (e.g. a measurement proxy +/// returned by both MeasurementContainer and MeasurementSubset). +template +struct ProxyTether { + using AliveFn = bool (*)(const pybind11::object&); + + pybind11::object owner; + Proxy proxy; + AliveFn aliveFn; + + /// Validate the container is still owned, then return the proxy. Throws + /// pybind11::value_error (a Python ValueError) if it was consumed. + const Proxy& checked() const { + validate(); + return proxy; + } + Proxy& checked() { + validate(); + return proxy; + } + + private: + void validate() const { + if (aliveFn == nullptr || !aliveFn(owner)) { + throw pybind11::value_error( + "proxy is no longer valid: its container was consumed (moved to the " + "whiteboard)"); + } + } +}; + +/// Build a read accessor for a tethered proxy: validate the tether, then call +/// `access` with the inner proxy. `Tether` is given explicitly at the call +/// site. +template +auto tetheredRead(Access access) { + return [access = std::move(access)](const Tether& self) { + return access(self.checked()); + }; +} + +/// Build a write accessor for a tethered proxy. `V` is the concrete value type, +/// given explicitly at the call site so pybind11 sees a concrete setter +/// signature (never `auto`). +template +auto tetheredWrite(Assign assign) { + return [assign = std::move(assign)](Tether& self, const V& value) { + assign(self.checked(), value); + }; +} + +/// A Python iterator that gates an underlying pybind11 iterator on the owner +/// still being alive. Used for containers whose iteration yields values/pairs +/// whose Python conversion is still registered (e.g. the flat multimaps). +template +struct CheckedIterator { + pybind11::object owner; + pybind11::iterator inner; + + pybind11::object next() { + if (!ownerAlive(owner)) { + throw pybind11::value_error( + "iterator is no longer valid: its container was consumed (moved to " + "the whiteboard)"); + } + return inner.attr("__next__")(); + } +}; + +/// Register a CheckedIterator type. MUST be called before any container's +/// `__iter__` that returns it, or pybind11 raises an unregistered-type error. +template +void bindCheckedIterator(pybind11::module_& m, const char* name) { + pybind11::class_>(m, name) + .def("__iter__", [](pybind11::object self) { return self; }) + .def("__next__", &CheckedIterator::next); +} + +/// A Python iterator over an index-accessible container that yields freshly +/// tethered proxies. Used for containers whose proxy type is itself bound as a +/// ProxyTether (so a plain make_iterator over raw proxies would hit an +/// unregistered type). `maker(owner, container, index)` builds the tethered +/// proxy for each element. +template +struct CheckedIndexIterator { + using Maker = std::function; + pybind11::object owner; + std::size_t index{0}; + Maker maker; + + pybind11::object next() { + Container* container = nullptr; + try { + container = &owner.cast(); + } catch (...) { + PyErr_Clear(); + throw pybind11::value_error( + "iterator is no longer valid: its container was consumed (moved to " + "the whiteboard)"); + } + if (index >= container->size()) { + throw pybind11::stop_iteration(); + } + return maker(owner, *container, index++); + } +}; + +/// Register a CheckedIndexIterator type. MUST be called before any container's +/// `__iter__` that returns it. +template +void bindCheckedIndexIterator(pybind11::module_& m, const char* name) { + pybind11::class_>(m, name) + .def("__iter__", [](pybind11::object self) { return self; }) + .def("__next__", &CheckedIndexIterator::next); +} + +} // namespace ActsPython