Unified Interface: Absolute Tolerance#1660
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1660 +/- ##
===========================================
- Coverage 84.31% 83.12% -1.19%
===========================================
Files 164 164
Lines 14328 14393 +65
===========================================
- Hits 12080 11964 -116
- Misses 2248 2429 +181 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
closes #1646 |
dweindl
left a comment
dweindl
left a comment
There was a problem hiding this comment.
Thanks, @PaulJonasJost. Good feature to add, but I think this requires a few changes:
It's currently unclear
- whether it's an absolute or relative tolerance. (included in the docstring, but not in the method name. we might want to support both in the future, so the method name should be unambiguous.)
- what the tolerance applies to. many gradient-based optimizers will support tolerances on the objective as well as its gradient, so it needs to become clear which one is specified here.
pypesto/optimize/optimizer.py
Outdated
| tol | ||
| Absolute tolerance for termination. | ||
| """ | ||
| self._set_option_tol(tol, "tol") |
There was a problem hiding this comment.
From what I remember, ipopt has quite complex termination criteria. While various tolerances are supported, I think just hitting this single value is insufficient for termination, so it might be a bit confusing. Not completely sure whether it should be added here or not.
There was a problem hiding this comment.
True, it says so in the IPOPT documentation with a quite lengthy passage. I would in general be fine with removing it, but i am not sure how to handle the supports_rel_tol later. I think we can say it does not directly support f_abs_tol but this one is a bit harder. Removed it for now!
# Conflicts: # pypesto/optimize/optimizer.py
pypesto/optimize/optimizer.py
Outdated
| f"Check supports_f_abs_tol() before calling set_f_abs_tol()." | ||
| ) | ||
|
|
||
| def _set_option_tol(self, tol: float, option_key: str) -> None: |
There was a problem hiding this comment.
class Optimizer does not have an options attribute, we should not try accessing it here. I'd leave validation to the optimizer and just set the options directly in each optimizer without this extra method.
| # We explicitly cast to float, as the IpoptOptimizer requires | ||
| # the provision of a float for the max_wall_time option. | ||
| self.options["max_wall_time"] = float(seconds) |
There was a problem hiding this comment.
that should not have happened. I think something got mixed up when i merged develop...
3 participants
Add tolerance to the unified interface. Some optimizers not supported, especially Dlib with epsilon, though epsilon is not strictly a tolerance!