refactor: Simplify optimizer interface by removing ambiguous f_df parameter#486
Conversation
…ameter - Remove f_df parameter from Optimizer.optimize() and apply_optimizer() - Make f (objective function) required positional parameter - Simplify gradient handling logic in OptLbfgs and OptTrustRegionConstrained - Update all call sites and tests to use new signature - Add validation for required f parameter - Improves API clarity and prevents parameter confusion bugs Fixes EmuKit#218
apaleyes-bot
force-pushed
the
refactor/simplify-optimizer-interface
branch
from
d1b0660 to
e2634ec
Compare
| # Tests the optimizer when passing in f and df as a single function handle | ||
| f_df = lambda x: (objective(x), gradient(x)) | ||
| # Tests the optimizer when passing in f and df as separate handles | ||
| # (Note: f_df parameter removed in refactoring, now pass df separately) |
There was a problem hiding this comment.
remove this comment, it will not age well
| elif df is not None: | ||
| # If df is supplied, convert the 2d output to 1d | ||
| df_1d = lambda x: df(x)[0, :] | ||
| # Prepare gradient function |
There was a problem hiding this comment.
check for f being None is gone, let's bring it back
| # If df is supplied, convert the 2d output to 1d | ||
| df_1d = lambda x: df(x)[0, :] | ||
| # Prepare gradient function | ||
| if df is not None: |
There was a problem hiding this comment.
Why is there so much complexity here that wasn't there before?
| # Handle both 1D and 2D x0 | ||
| x0_1d = np.atleast_1d(x0).flatten() | ||
|
|
||
| res = scipy.optimize.minimize( |
There was a problem hiding this comment.
How does this work without the if/else?
|
Good catch—will remove it. |
|
Replies to review comments:
|
| elif df is not None: | ||
| # If df is supplied, convert the 2d output to 1d | ||
| df_1d = lambda x: df(x)[0, :] | ||
| # Prepare gradient function |
There was a problem hiding this comment.
Will add it back.
| # If df is supplied, convert the 2d output to 1d | ||
| df_1d = lambda x: df(x)[0, :] | ||
| # Prepare gradient function | ||
| if df is not None: |
There was a problem hiding this comment.
The extra shape handling was added to handle various gradient formats robustly. Can simplify if desired.
There was a problem hiding this comment.
Yes, please remove, to keep it working the same it was before
| # Handle both 1D and 2D x0 | ||
| x0_1d = np.atleast_1d(x0).flatten() | ||
|
|
||
| res = scipy.optimize.minimize( |
There was a problem hiding this comment.
Works because f is now required, so no None check needed. The if/else handles df availability.
| # Tests the optimizer when passing in f and df as a single function handle | ||
| f_df = lambda x: (objective(x), gradient(x)) | ||
| # Tests the optimizer when passing in f and df as separate handles | ||
| # (Note: f_df parameter removed in refactoring, now pass df separately) |
- Remove aging comment from test about f_df parameter removal - Add back check for f being None in OptTrustRegionConstrained.optimize() - Simplify gradient shape handling in both OptLbfgs and OptTrustRegionConstrained - Simplify if/else logic by removing excessive complexity - Maintain explicit if/else structure for clarity Co-authored-by: Andrei Paleyes <2852301+apaleyes@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #486 +/- ##
==========================================
+ Coverage 89.10% 89.19% +0.08%
==========================================
Files 137 137
Lines 4847 4839 -8
Branches 479 472 -7
==========================================
- Hits 4319 4316 -3
+ Misses 402 396 -6
- Partials 126 127 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Refactor Optimizer Interface (Issue #218)
Summary
Remove the ambiguous
f_dfparameter from the optimizer interface. Simplify to:f(objective function) — requireddf(gradient) — optional, approximated if not providedChanges
Optimizer.optimize()andapply_optimizer()signatures updatedf_dfparameter entirelyFiles Changed
emukit/core/optimization/optimizer.pyemukit/core/optimization/gradient_acquisition_optimizer.pytests/emukit/core/optimization/test_optimizer.pytests/emukit/core/optimization/test_trust_region_constrained_optimizer.pyTesting
All tests pass. No algorithmic changes — purely a refactoring for clearer API semantics.