Over-approximations return new object#57
Conversation
Josh Robbins (jrobbins11)
commented
Apr 12, 2026
- Changing all over-approximation methods to return a new object rather than modify the object in-place
- Adding unit tests to verify over-approximation methods working as expected
There was a problem hiding this comment.
Pull request overview
This PR changes the over-approximation API to return new set objects (instead of mutating in-place) and adds test fixtures + unit tests to validate over-approximation behavior across Zono/ConZono/HybZono in both the Python and C++ test suites.
Changes:
- Update
ConZono::constraint_reduction()(C++ and Python bindings/stubs/docs) to return a new object. - Add new overapproximation test data and new C++/Python unit tests exercising
reduce_order,constraint_reduction,to_zono_approx, andconvex_relaxation. - Regenerate/update published docs to reflect new return types.
Reviewed changes
Copilot reviewed 30 out of 33 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-data/overapproximation/rand_zono.json | Adds randomized zonotope fixture for overapprox tests. |
| test/test-data/overapproximation/rand_hybzono.json | Adds randomized hybrid zonotope fixture for overapprox tests. |
| test/test-data/overapproximation/rand_conzono.json | Adds randomized constrained zonotope fixture for overapprox tests. |
| test/python-tests/unit_tests.py | Adds Python unit test coverage for over-approximation methods. |
| test/cpp-tests/src/test_overapproximation.cpp | Adds a new C++ unit test binary for over-approximation methods. |
| test/CMakeLists.txt | Registers the new C++ overapproximation test target. |
| src/ConZono.cpp | Changes ConZono::constraint_reduction() to return a new object and be const. |
| python/zonoopt/_core.pyi | Updates Python typing stub for constraint_reduction() return type/documentation. |
| python/src/zonoopt_py.cpp | Exposes updated constraint_reduction() return semantics in the Python module docs/binding. |
| include/zonoopt/Zono.hpp | Adds an override for constraint_reduction() in Zono. |
| include/zonoopt/EmptySet.hpp | Updates constraint_reduction() override to return a new object. |
| include/zonoopt/ConZono.hpp | Updates constraint_reduction() signature to return unique_ptr and be const. |
| docs/python/build/html/zonoopt.html | Regenerated Python docs to reflect new return type and docs. |
| docs/C++/html/Zono_8hpp_source.html | Regenerated C++ docs (Zono header source view). |
| docs/C++/html/search/functions_2.js | Regenerated C++ docs search index for function signatures. |
| docs/C++/html/search/all_2.js | Regenerated C++ docs search index for symbol changes. |
| docs/C++/html/index.html | Regenerated C++ docs landing page references/line numbers. |
| docs/C++/html/functions_func_c.html | Regenerated C++ docs function index for “c”. |
| docs/C++/html/functions_c.html | Regenerated C++ docs member function index for “c”. |
| docs/C++/html/EmptySet_8hpp_source.html | Regenerated C++ docs (EmptySet header source view). |
| docs/C++/html/ConZono_8hpp_source.html | Regenerated C++ docs (ConZono header source view). |
| docs/C++/html/classZonoOpt_1_1Zono.html | Regenerated C++ docs (Zono class page). |
| docs/C++/html/classZonoOpt_1_1Zono-members.html | Regenerated C++ docs (Zono members page). |
| docs/C++/html/classZonoOpt_1_1Point.html | Regenerated C++ docs (Point class page). |
| docs/C++/html/classZonoOpt_1_1Point-members.html | Regenerated C++ docs (Point members page). |
| docs/C++/html/classZonoOpt_1_1EmptySet.html | Regenerated C++ docs (EmptySet class page). |
| docs/C++/html/classZonoOpt_1_1EmptySet-members.html | Regenerated C++ docs (EmptySet members page). |
| docs/C++/html/classZonoOpt_1_1ConZono.html | Regenerated C++ docs (ConZono class page). |
| docs/C++/html/classZonoOpt_1_1ConZono-members.html | Regenerated C++ docs (ConZono members page). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // make sure conzono isn't empty (interval check) | ||
| for (int j = 0; j < this->nG; ++j) | ||
| for (int j = 0; j < Z->nG; ++j) | ||
| { | ||
| if (E.get_element(j).is_empty()) | ||
| throw std::runtime_error("ConZono constraint reduction: set is empty"); | ||
| return std::make_unique<EmptySet>(Z->n); | ||
| } |
There was a problem hiding this comment.
constraint_reduction() used to throw when the interval contractor proves the set empty; it now returns an EmptySet instead. This is a behavioral/API change that isn’t reflected in the method docs (C++/Python) and may break callers that rely on catching the exception. Consider either preserving the exception behavior or explicitly documenting that an EmptySet may be returned when emptiness is detected.
| if (!Z->is_zono()) | ||
| { | ||
| throw std::invalid_argument("Expected set to be a zonotope for reduce order test."); | ||
| } | ||
|
|
||
| std::unique_ptr<Zono> Z_zono (dynamic_cast<Zono*>(Z.release())); | ||
|
|
There was a problem hiding this comment.
The dynamic_cast is performed after Z.release(). If the cast unexpectedly fails, the raw pointer is leaked. Prefer checking/casting while the unique_ptr still owns the object (e.g., cast Z.get() and only release() after validating), or use a safer ownership transfer pattern.
| // convert to ConZono | ||
| if (!Z->is_conzono()) | ||
| { | ||
| throw std::invalid_argument("Expected set to be a constrained zonotope for constraint reduction test."); | ||
| } | ||
|
|
||
| std::unique_ptr<ConZono> Z_conzono (dynamic_cast<ConZono*>(Z.release())); | ||
|
|
There was a problem hiding this comment.
Same issue as above: dynamic_cast after release() can leak the pointer if the cast fails. Cast/validate before releasing ownership (or restructure to avoid release() entirely).
| // convert to ConZono | ||
| if (!Z->is_conzono()) | ||
| { | ||
| throw std::invalid_argument("Expected set to be a constrained zonotope for zonotope approximation test."); | ||
| } | ||
|
|
||
| std::unique_ptr<ConZono> Z_conzono (dynamic_cast<ConZono*>(Z.release())); | ||
|
|
There was a problem hiding this comment.
Same issue as above: dynamic_cast after release() can leak the pointer if the cast fails. Cast/validate before releasing ownership (or restructure to avoid release() entirely).
| assert Z.is_zono(), '_test_reduce_order: expected set to be a zonotope' | ||
|
|
||
| p = np.zeros(Z.get_n()) | ||
| p_proj = Z.project_point(p) | ||
|
|
||
| Z_r = Z.reduce_order(10) | ||
|
|
There was a problem hiding this comment.
These tests validate the over-approximation property, but they don’t verify the new contract that reduce_order() returns a new object without mutating Z. Add an assertion that Z is unchanged after the call (e.g., save Z_before = Z.copy() and compare with TestUtilities.eq_hzs(Z, Z_before)).
| assert Z.is_zono(), '_test_reduce_order: expected set to be a zonotope' | |
| p = np.zeros(Z.get_n()) | |
| p_proj = Z.project_point(p) | |
| Z_r = Z.reduce_order(10) | |
| assert Z.is_zono(), '_test_reduce_order: expected set to be a zonotope' | |
| Z_before = Z.copy() | |
| p = np.zeros(Z.get_n()) | |
| p_proj = Z.project_point(p) | |
| Z_r = Z.reduce_order(10) | |
| assert TestUtilities.eq_hzs(Z, Z_before), '_test_reduce_order: reduce_order mutated the original zonotope' |
| p_proj = Z.project_point(p) | ||
|
|
||
| Z_cr = Z.constraint_reduction() | ||
|
|
There was a problem hiding this comment.
These tests validate the over-approximation property, but they don’t verify the new contract that constraint_reduction() returns a new object without mutating Z. Add an assertion that Z is unchanged after the call (e.g., save Z_before = Z.copy() and compare with TestUtilities.eq_hzs(Z, Z_before)).
| p_proj = Z.project_point(p) | |
| Z_cr = Z.constraint_reduction() | |
| p_proj = Z.project_point(p) | |
| Z_before = Z.copy() | |
| Z_cr = Z.constraint_reduction() | |
| assert TestUtilities.eq_hzs(Z, Z_before), '_test_constraint_reduction: constraint_reduction mutated the original constrained zonotope' |
|
|
||
| p = np.zeros(Z.get_n()) | ||
| p_proj = Z.project_point(p) | ||
|
|
||
| Z_approx = Z.to_zono_approx() | ||
|
|
There was a problem hiding this comment.
These tests validate the over-approximation property, but they don’t verify the new contract that to_zono_approx() returns a new object without mutating Z. Add an assertion that Z is unchanged after the call (e.g., save Z_before = Z.copy() and compare with TestUtilities.eq_hzs(Z, Z_before)).
| p = np.zeros(Z.get_n()) | |
| p_proj = Z.project_point(p) | |
| Z_approx = Z.to_zono_approx() | |
| Z_before = Z.copy() | |
| p = np.zeros(Z.get_n()) | |
| p_proj = Z.project_point(p) | |
| Z_approx = Z.to_zono_approx() | |
| assert TestUtilities.eq_hzs(Z, Z_before), '_test_to_zono_approx: original constrained zonotope was mutated' |
| Z = zono.from_json(str(overapprox_folder / 'rand_hybzono.json')) | ||
|
|
||
| p = np.zeros(Z.get_n()) | ||
| p_proj = Z.project_point(p) | ||
|
|
||
| Z_relax = Z.convex_relaxation() | ||
|
|
There was a problem hiding this comment.
These tests validate the over-approximation property, but they don’t verify the new contract that convex_relaxation() returns a new object without mutating Z. Add an assertion that Z is unchanged after the call (e.g., save Z_before = Z.copy() and compare with TestUtilities.eq_hzs(Z, Z_before)).
| Z = zono.from_json(str(overapprox_folder / 'rand_hybzono.json')) | |
| p = np.zeros(Z.get_n()) | |
| p_proj = Z.project_point(p) | |
| Z_relax = Z.convex_relaxation() | |
| Z = zono.from_json(str(overapprox_folder / 'rand_hybzono.json')) | |
| Z_before = Z.copy() | |
| p = np.zeros(Z.get_n()) | |
| p_proj = Z.project_point(p) | |
| Z_relax = Z.convex_relaxation() | |
| assert TestUtilities.eq_hzs(Z, Z_before), '_test_convex_relaxation: convex_relaxation mutated the original set' |
|
|
||
| std::unique_ptr<ConZono> constraint_reduction() const override | ||
| { | ||
| return std::make_unique<Zono>(*this); |
There was a problem hiding this comment.
This override always returns a Zono copy. For classes derived from Zono (e.g., Point), calling constraint_reduction() will slice the dynamic type and return a Zono instead of preserving the derived type (which can break is_point() checks / Python type dispatch). Consider overriding constraint_reduction() in derived classes like Point, or refactoring the base implementation to clone-preserve the dynamic type (e.g., via clone() + downcast) when no reduction is needed.
| return std::make_unique<Zono>(*this); | |
| return std::unique_ptr<ConZono>(static_cast<ConZono*>(this->clone())); |