-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement FD step size adaptation #671
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #671 +/- ##
===========================================
+ Coverage 88.16% 90.33% +2.16%
===========================================
Files 79 96 +17
Lines 5257 6363 +1106
===========================================
+ Hits 4635 5748 +1113
+ Misses 622 615 -7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good, some minor stuff left
CONSTANT = "constant" | ||
SPACE = "space" | ||
STEPS = "steps" | ||
UPDATE_CONDITIONS = [CONSTANT, SPACE, STEPS] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> constants.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather leave these here, s.t. one has to call them pypesto.FDDelta.CONSTANT
, as they are only used in this specific context.
assert np.allclose(val, v, atol=atol, rtol=rtol), attr | ||
# cannot completely coincide | ||
assert (val != v).any(), attr | ||
assert np.allclose(val, val_fd, atol=atol, rtol=rtol), attr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we maybe double check, whether we achieve better accuracies when using adaptive FDs than when using static FDs? Maybe even down to 1e-5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is 1e-6, which already gives quite a good approximation (automatically chosen are 1e-7 to 1e-8), so on this problem not much gain, rather a "automatic detection does not screw things up" check.
Step sizes adaptation for finite differences, realized via a
pypesto.objective.finite_difference.FDDelta
class.Also, some simplifications of grad+hess+sres derivative calculations (i.e. rather interpret those as 1st or 2nd order derivatives with code reuse).
Reference: https://github.com/ICB-DCM/PESTO/blob/master/private/getStepSizeFD.m