Implementation of hidden constraints handling#377
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements hidden (aka unknown or crash) constraints handling in the EGObox optimizer, addressing issue #376. The implementation is based on the approach described in Bussemaker et al. (2024) paper. When objective function evaluations fail (return NaN), the optimizer can now either reject the failed point (default) or impute the objective value using a penalized surrogate prediction.
Changes:
- Added a new
FailsafeStrategyenum with two variants:Rejection(default) andImputation - Implemented NaN detection and filtering in both initial DOE and optimization iterations
- Added penalized prediction computation (
pred + varfor objective) to handle failed points during imputation - Extended state management to track failed points throughout optimization
- Added comprehensive tests and examples demonstrating the new functionality
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
crates/ego/src/types.rs |
Defines FailsafeStrategy enum for hidden constraints handling |
crates/ego/src/solver/egor_config.rs |
Adds failsafe_strategy configuration option |
crates/ego/src/solver/egor_state.rs |
Adds x_fail field to track failed evaluation points and helper methods |
crates/ego/src/solver/egor_solver.rs |
Filters NaN values from initial DOE and initializes failed points tracking |
crates/ego/src/solver/solver_impl.rs |
Implements penalized prediction imputation logic for failed points |
crates/ego/src/solver/solver_computations.rs |
Adds compute_penalized_point method for crash handling |
crates/ego/src/solver/trego.rs |
Integrates failsafe strategy into TREGO algorithm |
crates/ego/src/utils/misc.rs |
Implements filter_nans, usable_data, and updated update_data functions |
crates/ego/src/utils/portfolio.rs |
Updates portfolio signature to include penalized values |
crates/ego/src/solver/solver_infill_optim.rs |
Adds constructor for InfillOptProblem |
crates/ego/src/egor.rs |
Adds comprehensive tests for failsafe strategies |
crates/ego/src/lib.rs |
Updates documentation with reference to Bussemaker2024 paper |
python/src/types.rs |
Defines Python FailsafeStrategy enum |
python/src/lib.rs |
Registers FailsafeStrategy class in Python module |
python/src/egor.rs |
Adds failsafe_strategy parameter to Egor Python class |
python/egobox/egobox.pyi |
Updates type stubs with FailsafeStrategy enum and parameter |
python/egobox/tests/test_egor.py |
Adds test for constrained Branin with NaN handling, adjusts tolerances |
python/egobox/examples/branin_nans.py |
New example demonstrating hidden constraints via NaN returns |
python/egobox/examples/branin_fcstr.py |
New example showing standard constraint handling for comparison |
README.md |
Adds reference to Bussemaker2024 paper |
crates/ego/src/solver/solver_impl.rs
Outdated
|
|
||
| // update_data( | ||
| // &mut x_dat, &mut y_dat, &mut c_dat, x_new, &y_new, &c_new, None, | ||
| // ); |
There was a problem hiding this comment.
There is commented-out code that should be removed. This dead code appears to be leftover from development and serves no purpose in the final implementation.
| // update_data( | |
| // &mut x_dat, &mut y_dat, &mut c_dat, x_new, &y_new, &c_new, None, | |
| // ); |
python/egobox/tests/test_egor.py
Outdated
| return branin(np.atleast_2d(xi))[0, 0] | ||
|
|
||
| res = np.apply_along_axis(branin_constraint_nans, 1, x).reshape(-1, 1) | ||
| print(x, res) |
There was a problem hiding this comment.
Debug print statement should be removed from test code. This print statement at line 393 appears to be leftover from debugging and should be removed for cleaner test output.
| print(x, res) |
crates/ego/src/lib.rs
Outdated
| //! * Theta bounds are implemented as in \[[Appriou2023](#Appriou2023)\] | ||
| //! * Logirithm of Expected Improvement is implemented as in \[[Ament2025](#Ament2025)\] | ||
| //! * Logarithm of Expected Improvement is implemented as in \[[Ament2025](#Ament2025)\] | ||
| //! * Hidden constraints handling are implemented as in \[[Bussemaker2024](#Bussemaker2024)\] |
There was a problem hiding this comment.
Grammatical error in documentation: "are" should be "is". The phrase should read "Hidden constraints handling is implemented as in" rather than "are implemented as in" since "handling" is singular.
| //! * Hidden constraints handling are implemented as in \[[Bussemaker2024](#Bussemaker2024)\] | |
| //! * Hidden constraints handling is implemented as in \[[Bussemaker2024](#Bussemaker2024)\] |
| /// * First element of the tuple is the objective function values | ||
| /// * Second element of the tuple is the penalized objective function values | ||
| /// used in case of true evaluation crash | ||
| /// This a the penalized prediction (pred + var) for objective |
There was a problem hiding this comment.
Typo in comment: "This a the" should be "This is the". The comment should read "This is the penalized prediction" instead of "This a the penalized prediction".
| /// This a the penalized prediction (pred + var) for objective | |
| /// This is the penalized prediction (pred + var) for objective |
| unittest.main( | ||
| defaultTest=["TestEgor.test_constrained_branin_with_nans"], exit=False | ||
| ) |
There was a problem hiding this comment.
The defaultTest parameter should be removed or commented out before merging to main. The change on lines 413-415 sets a specific test as the default test to run, which is useful during development but should not be committed to the main branch as it prevents running the full test suite by default.
| unittest.main( | |
| defaultTest=["TestEgor.test_constrained_branin_with_nans"], exit=False | |
| ) | |
| unittest.main(exit=False) |
| self.assertAlmostEqual(18.935, res.x_opt[0], delta=5e-2) | ||
|
|
||
| def test_xsinx_with_reclustering(self): | ||
| egor = egx.Egor([[0.0, 25.0]], seed=42, gp_config=egx.GpConfig(n_clusters=0)) | ||
| res = egor.minimize(xsinx, max_iters=20) | ||
| print(f"Optimization f={res.y_opt} at {res.x_opt}") | ||
| self.assertAlmostEqual(-15.125, res.y_opt[0], delta=1e-3) | ||
| self.assertAlmostEqual(18.935, res.x_opt[0], delta=1e-2) | ||
| self.assertAlmostEqual(18.935, res.x_opt[0], delta=5e-2) |
There was a problem hiding this comment.
Test tolerance relaxation should be justified or reverted. The delta tolerance was increased from 1e-2 to 5e-2 for x_opt assertions in the xsinx tests (lines 169 and 176). This relaxation of test tolerances should either be justified with a comment explaining why it's necessary (e.g., if the hidden constraints handling affects convergence behavior) or reverted if it's an unintended change. Relaxing test tolerances without justification can mask actual regressions.
| print(f"Constraint violated at {xi}, returning NaN") | ||
| return np.nan | ||
| else: | ||
| print(branin_forrester(np.atleast_2d(xi))) | ||
| return branin_forrester(np.atleast_2d(xi)) |
There was a problem hiding this comment.
Debug print statements should be removed from the example. The print statements at lines 54 and 57 within the constraint evaluation function appear to be debug traces and should be removed for cleaner output in the example.
| print(f"Constraint violated at {xi}, returning NaN") | |
| return np.nan | |
| else: | |
| print(branin_forrester(np.atleast_2d(xi))) | |
| return branin_forrester(np.atleast_2d(xi)) | |
| return np.nan | |
| else: | |
| value = branin_forrester(np.atleast_2d(xi)) | |
| return value |
crates/ego/src/solver/solver_impl.rs
Outdated
| let y_pred = self | ||
| .compute_penalized_point(&xfail, models[0].as_ref(), &models[1..]) | ||
| .unwrap(); |
There was a problem hiding this comment.
Use of unwrap() without error handling could cause panic. The call to compute_penalized_point() at line 679 uses unwrap(), which could panic if the prediction fails. Since this is in a critical path (handling failed points during optimization), consider propagating the error or handling it more gracefully. The existing pattern in the codebase (line 808-810) shows that Result can be returned and handled properly.
| def test_g24_with_fcstrs(self): | ||
| n_doe = 5 | ||
| max_iters = 20 | ||
| max_iters = 5 |
There was a problem hiding this comment.
Reduction in test iterations reduces test coverage. The max_iters was reduced from 20 to 5 for the g24_with_fcstrs test (line 324). This significant reduction in the number of optimization iterations may reduce the test's ability to catch convergence issues or regressions. If this change was made to speed up tests, consider whether the reduced coverage is acceptable or if this test should be split into a quick smoke test and a more thorough integration test.
This PR introduces hidden (aka unknown or crash) constraints handling
The implemention is based on hidden constraints handling described in this paper
Egor manages function evaluation crashes in two ways:
Rejection(default): where objective value and corresponding are is simply discardedImputation: the objective is an imputed value with penalized prediction from the objective function surrogaterelates to #376