Add optimization test with FD derivatives - #487
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
=======================================
Coverage 54.22% 54.22%
=======================================
Files 1 1
Lines 225 225
=======================================
Hits 122 122
Misses 103 103 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Which one should be merged first between this PR and #486 ? Both add the same two new files, does it make sense attach them to one PR over the other? |
|
This is on top of #486, let's wait for that one first. |
| # Complex step divides by the imaginary part of the step, so a purely | ||
| # real step would silently yield NaN gradients. | ||
| if self.sensType == "cs" and np.imag(self.sensStep) == 0: | ||
| raise ValueError(f"The complex step size must have a nonzero imaginary part, got {self.sensStep}.") |
There was a problem hiding this comment.
The docstring says that sensStep is the step size, and should be a float. So we need to either
- Update the docstring
- Change the implementation so that we take in a float and do
self.sensStep = 1j * sensStepwhen using complex-step
Option 2 would be my slight preference.
There was a problem hiding this comment.
Also, if we keep the implementation as is and update the docstring, shouldn't we also check that sensStep is purely imaginary (i.e np.real(self.sensStep) == 0?)
There was a problem hiding this comment.
I would prefer not to change any code behaviour (other than additional error checking as done here). The "correct" path can be updated later on if we want to, but definitely require a separate PR and more documentation. I will update the docstring to be more clear, but yes as coded the user needs to pass the actual complex perturbation. In practice, the default of 1e-40j should work for almost all scenarios.
|
This PR should be merged after #486, I will rebase it after that one is merged. |
| product(ALL_OPTIMIZERS, ["fd", "fdr", "cd", "cdr", "cs"]), | ||
| name_func=lambda f, n, p: f"{f.__name__}_{p.args[0]}_{p.args[1]}", | ||
| ) | ||
| def test_optimization_approx_deriv(self, optName, sens): |
There was a problem hiding this comment.
This is the only new test added here, the rest are from the other branch (the diff will look better once the other PR is merged).
Updated sensitivity type descriptions to use lowercase and clarified the sensStep parameter details.
3 participants
Purpose
Addresses part of #256.
Expected time until merged
A few days.
Type of change
Testing
Checklist
ruff checkandruff formatto make sure the Python code adheres to PEP-8 and is consistently formattedfprettifyor C/C++ code withclang-formatas applicable