Always populating solution structure#62
Conversation
There was a problem hiding this comment.
Pull request overview
This PR ensures OptSolution is consistently populated even for set operations that don’t require numerical optimization (e.g., zonotopes/points/empty set), and adds regression tests to confirm Zono and equivalent ConZono calls return matching support values and factor solutions.
Changes:
- Populate
OptSolutioninZono,Point, andEmptySetoperations that previously didn’t fill solution fields. - For
Zono::support, compute and return the support-achieving factor vectorzin the solution struct. - Add Python and C++ unit tests comparing
ZonovsConZonosupport values and returned factors.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Zono.cpp |
Populates OptSolution for is_empty, bounding_box, and fills z/x/u for support. |
src/Point.cpp |
Populates OptSolution for point operations (optimize_over, project_point, support, contains_point, bounding_box). |
src/EmptySet.cpp |
Ensures OptSolution is allocated and marked infeasible for empty-set operations; marks complement as feasible/converged. |
include/zonoopt/Zono.hpp |
Updates overridden method signatures to include named sol parameter. |
include/zonoopt/Point.hpp |
Updates overridden method signatures to include named sol parameter. |
include/zonoopt/EmptySet.hpp |
Updates overridden method signatures to include named sol parameter (formatting/consistency). |
test/python-tests/unit_tests.py |
Adds a unit test checking Zono vs ConZono support value and solution factor consistency. |
test/cpp-tests/src/test_support.cpp |
Adds a unit test checking Zono vs ConZono support value and solution factor consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| zono_float h = d.dot(this->c); | ||
| const Eigen::Matrix<zono_float, -1, -1> Gd = this->G.toDense(); | ||
| for (int i = 0; i < this->nG; ++i) | ||
| { | ||
| h += std::abs(d.dot(Gd.col(i))); | ||
| } | ||
|
|
||
| // fill in solution struct if requested | ||
| if (sol) | ||
| { | ||
| *sol = std::make_shared<OptSolution>(); // init w/ default fields | ||
|
|
||
| // compute factors for support | ||
| (*sol)->z.resize(this->nG); // init | ||
| for (int i = 0; i < this->nG; ++i) | ||
| { | ||
| const zono_float dG = d.dot(Gd.col(i)); | ||
| if (dG > zono_eps) | ||
| { | ||
| (*sol)->z(i) = one; |
There was a problem hiding this comment.
In Zono::do_support, when sol is provided the code computes d.dot(Gd.col(i)) twice per generator (once for h and again when filling sol->z). Consider computing G.transpose() * d once and using it to accumulate h and populate z in a single pass (and potentially avoid densifying G).
| if (sol) | ||
| { | ||
| *sol = std::make_shared<OptSolution>(); // init w/ default fields | ||
| (*sol)->infeasible = false; | ||
| (*sol)->converged = true; | ||
| (*sol)->primal_residual = zero; | ||
| (*sol)->dual_residual = zero; | ||
| } |
There was a problem hiding this comment.
The OptSolution initialization block (allocate + set infeasible/converged/primal_residual/dual_residual) is duplicated across multiple Point operations. Consider factoring this into a small helper to reduce duplication and keep the populated fields consistent across methods.
| if (sol) | ||
| { | ||
| (*solution)->infeasible = true; | ||
| *sol = std::make_shared<OptSolution>(); // init w/ default fields | ||
| (*sol)->infeasible = true; | ||
| (*sol)->converged = false; | ||
| (*sol)->primal_residual = std::numeric_limits<zono_float>::infinity(); | ||
| (*sol)->dual_residual = std::numeric_limits<zono_float>::infinity(); | ||
| } |
There was a problem hiding this comment.
EmptySet now allocates and populates an OptSolution, but the same “infeasible solution” initialization block is repeated in every overridden method. Consider centralizing this into a single helper (e.g., fill_infeasible_solution(std::shared_ptr<OptSolution>* sol)) to reduce duplication and keep the flags/residuals consistent across operations.
… rather than randomly generating
Josh Robbins (jrobbins11)
deleted the
feature/populate-opt-solution-fields
branch
Uh oh!
There was an error while loading. Please reload this page.