-
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
Adding a helper method to check model gradients before optimization. #690
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #690 +/- ##
============================================
+ Coverage 34.58% 90.10% +55.52%
============================================
Files 96 96
Lines 6466 6507 +41
============================================
+ Hits 2236 5863 +3627
+ Misses 4230 644 -3586
Continue to review full report at Codecov.
|
Co-authored-by: Paul Jonas Jost <70631928+PaulJonasJost@users.noreply.github.com>
Co-authored-by: Paul Jonas Jost <70631928+PaulJonasJost@users.noreply.github.com>
…into check_and_log_gradients
Added your suggestion @PaulJonasJost - thanks for the pointers. |
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.
Generally useful. I am only wondering if it might be better to move it to a different place . It doesn't really depend on PEtab. pypesto.Problem
? @yannikschaelte ?
free_indices, | ||
multi_eps=[1e-3, 1e-4, 1e-5], | ||
mode=mode)) | ||
except (RuntimeError, ValueError): |
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.
Short comment why/when this is expected to happen?
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.
Added in commit, see below.
pypesto/petab/importer.py
Outdated
@@ -78,6 +80,38 @@ def from_yaml(yaml_config: Union[dict, str], | |||
output_folder=output_folder, | |||
model_name=model_name) | |||
|
|||
def check_and_log_gradients(self, RTOL: float = 1e-2, ATOL: float = 1e-3): |
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.
Please use lower-case argument names
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.
Corrected.
pypesto/petab/importer.py
Outdated
"""Check and log if gradients are compatible with gradient-based optimizers | ||
Parameters |
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.
"""Check and log if gradients are compatible with gradient-based optimizers | |
Parameters | |
"""Check and log if gradients match finite differences | |
Parameters |
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.
Changed the documentation brief.
Perhaps the def check_and_log_gradients(self, x: np.ndarray = None, *args, **kwargs):
if x is None and 'petab_problem' in dir(self.amici_object_builder):
x = self.amici_object_builder.petab_problem.x_nominal_scaled
return super(ObjectiveBase, self).check_and_log_gradients(x=x, *args, **kwargs) |
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.
👍
Mostly suggestions, feel free to ignore, thanks for the helper method!
pypesto/petab/importer.py
Outdated
try: | ||
dfs.append(objective.check_grad_multi_eps( | ||
free_indices, | ||
multi_eps=[1e-3, 1e-4, 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.
Could remove multi_eps
from here (check_grad_multi_eps
already has a default multi_eps
) and replace with *args
and **kwargs
, so that the user has more control over the gradient check.
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.
Done.
pypesto/petab/importer.py
Outdated
modes = [MODE_FUN, MODE_RES] | ||
for mode in modes: |
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.
This loop over modes
could be by default, but also allow users to only specify one of MODE_FUN
or MODE_RES
, to reduce computation (perhaps not a significant issue except for large problems).
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.
Okay, default is to consider both modes but give the user control over it.
pypesto/petab/importer.py
Outdated
multi_eps=[1e-3, 1e-4, 1e-5], | ||
mode=mode)) | ||
except (RuntimeError, ValueError): | ||
dfs.append(dfs[0]) |
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.
Not sure, what does this achieve? Also, it will raise an error if dfs == []
too.
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.
If this is to ensure len(dfs) == 2
for the return
statement, then see my suggestion there for an alternative.
pypesto/petab/importer.py
Outdated
objective.amici_solver.setSensitivityMethod( | ||
amici.SensitivityMethod_forward) | ||
objective.amici_solver.setAbsoluteTolerance(1e-10) | ||
objective.amici_solver.setRelativeTolerance(1e-12) |
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.
These hard-coded problem-dependent settings should instead be default values that the user can customize (some models will fail at these tolerances, or may be overly computationally-expensive with forward sensitivities). Alternatively, the function could accept an objective that is already customized by the user.
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.
Made these hardcoded values (1e-10 and 1e-12) a user input, allow for pre-configured objective function as user input as well.
RTOL: relative error tolerance (default 1e-2) | ||
ATOL: absolute error tolerance (default 1e-3) | ||
""" | ||
par = np.asarray(self.petab_problem.x_nominal_scaled) |
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.
Could be by default but also accept a user-supplied vector.
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.
Made as default and allow user defined tolerances.
pypesto/petab/importer.py
Outdated
return np.all((dfs[0].rel_err.values < RTOL) | | ||
(dfs[0].abs_err.values < ATOL) | | ||
(dfs[1].rel_err.values < RTOL) | | ||
(dfs[1].abs_err.values < ATOL)) |
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.
return np.all((dfs[0].rel_err.values < RTOL) | | |
(dfs[0].abs_err.values < ATOL) | | |
(dfs[1].rel_err.values < RTOL) | | |
(dfs[1].abs_err.values < ATOL)) | |
# Gradients are correct if any of the error types are within their | |
# respective tolerance, for any of the modes. | |
return any([ | |
any([ | |
np.all(mode_df.rel_err.values < RTOL), | |
np.all(mode_df.abs_err.values < ATOL), | |
]) | |
for mode_df in dfs | |
]) |
For another suggestion.
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.
Merged.
test/petab/test_petab_import.py
Outdated
@@ -76,6 +76,13 @@ def test_3_optimize(self): | |||
self.assertTrue(np.isfinite( | |||
result.optimize_result.get_for_key('fval')[0])) | |||
|
|||
def test_4_gradients(self): |
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.
def test_4_gradients(self): | |
def test_check_and_log_gradients(self): |
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.
Ah I thought you want to enumerate the test in the class, as the previous tests are enumerated. Changed!
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.
Ah, didn't notice sorry, fine as before then.
pypesto/petab/importer.py
Outdated
@@ -1,4 +1,6 @@ | |||
from pypesto.objective.constants import MODE_FUN, MODE_RES |
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.
from pypesto.objective.constants import MODE_FUN, MODE_RES | |
from ..objective.constants import MODE_FUN, MODE_RES |
Relative imports are a few lines below, could move this there.
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.
Cleaned up imports.
pypesto/petab/importer.py
Outdated
@@ -78,6 +80,38 @@ def from_yaml(yaml_config: Union[dict, str], | |||
output_folder=output_folder, | |||
model_name=model_name) | |||
|
|||
def check_and_log_gradients(self, RTOL: float = 1e-2, ATOL: float = 1e-3): | |||
"""Check and log if gradients are compatible with gradient-based optimizers |
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.
What is the log
functionality of this method?
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.
You are right, the logging is implictly done if log level is specified to check_grad_multi_eps
.
@dilpath @dweindl @PaulJonasJost please compare if I added all your suggestions. |
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.
Generally useful. I am only wondering if it might be better to move it to a different place . It doesn't really depend on PEtab.
pypesto.Problem
? @yannikschaelte ?Perhaps the
ObjectiveBase
class, where the gradient check methods are implemented? However, the convenience of automatically usingpetab_problem.x_nominal_scaled
as the test vector would then be lost. This could still be possible by redefiningcheck_and_log_gradients
inAmiciObjective
, as e.g.:def check_and_log_gradients(self, x: np.ndarray = None, *args, **kwargs): if x is None and 'petab_problem' in dir(self.amici_object_builder): x = self.amici_object_builder.petab_problem.x_nominal_scaled return super(ObjectiveBase, self).check_and_log_gradients(x=x, *args, **kwargs)
sounds good to me, i.e. defining generically in the base class and for amici/petab allowing to use the reference parameter.
pypesto/petab/importer.py
Outdated
objAbsoluteTolerance: float = 1e-10, | ||
objRelativeTolerance: float = 1e-12, | ||
multi_eps=None, | ||
**kwargs |
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.
**kwargs | |
**kwargs, |
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.
Okay.
pypesto/petab/importer.py
Outdated
modes = [mode] | ||
|
||
if multi_eps is None: | ||
multi_eps = [1e-3, 1e-4, 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.
in ODE problems, we often find FDs of step size between 1e-1 and 1e-8 stable (see also https://github.com/ICB-DCM/pyPESTO/blob/develop/pypesto/objective/finite_difference.py#L66), so one could define a broader default regime.
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.
Changed.
pypesto/petab/importer.py
Outdated
"""Check if gradients match finite differences (FDs) | ||
Parameters |
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.
"""Check if gradients match finite differences (FDs) | |
Parameters | |
"""Check if gradients match finite differences (FDs) | |
Parameters |
pypesto/petab/importer.py
Outdated
---------- | ||
rtol: relative error tolerance | ||
atol: absolute error tolerance | ||
mode: unction values or residuals |
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.
mode: unction values or residuals | |
mode: function values or residuals |
pypesto/petab/importer.py
Outdated
objAbsoluteTolerance: float = 1e-10, | ||
objRelativeTolerance: float = 1e-12, |
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.
objAbsoluteTolerance: float = 1e-10, | |
objRelativeTolerance: float = 1e-12, | |
obj_absolute_tolerance: float = 1e-10, | |
obj_relative_tolerance: float = 1e-12, |
We use snake_case for everything but class names (PEP8). Please also adapt below.
|
||
Returns | ||
------- | ||
bool |
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 prefer using return type hints in the function signature instead of specifying it in the docstring.
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.
agreed, call it "match" or something
pypesto/petab/importer.py
Outdated
rtol: float = 1e-2, | ||
atol: float = 1e-3, | ||
mode: Literal = None, | ||
objective: Objective = None, |
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.
objective: Objective = None, | |
objective: Optional[Objective] = None, |
*args, | ||
rtol: float = 1e-2, | ||
atol: float = 1e-3, | ||
mode: Literal = None, |
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.
mode: Literal = None, | |
mode: Optional[Literal] = None, |
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't use plain Literal
in Optional
.
pypesto/petab/importer.py
Outdated
# fails for specified tolerances in forward sensitivities | ||
pass | ||
|
||
return any([ |
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.
return any([ | |
return all([ |
Shouldn't that be all
? If it fails for one mode, I'd consider that a failure.
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.
Yes it should be all, you are right.
pypesto/petab/importer.py
Outdated
except (RuntimeError, ValueError): | ||
# Might happen in case PEtab problem not well defined or | ||
# fails for specified tolerances in forward sensitivities | ||
pass |
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.
What will the function return in this case? Or will it raise during the check? I wouldn't not want it return True.
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.
Returning False
now instead of pass.
pypesto/petab/importer.py
Outdated
np.all(mode_df.rel_err.values < rtol), | ||
np.all(mode_df.abs_err.values < atol), |
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.
Hm, if one row passes only the relative tolerance and another row passes only the absolute tolerance, this report failure, right? This would be rather unexpected for me.
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.
So you suggest to make this an or
statement?
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.
np.all(mode_df.rel_err.values < rtol), | |
np.all(mode_df.abs_err.values < atol), | |
np.all((mode_df.rel_err.values < rtol) | (mode_df.abs_err.values < atol)), |
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.
Yes, that's what I just added.
To which class should the method be moved? |
Meanwhile it has an AMICI dependency, so |
Okay, maybe @yannikschaelte could clarify? Then I will make the changes accordingly. Thank you! |
…into check_and_log_gradients
What amici dependency do you mean? The amici specific forward sensi + tolerance instruction obviously only makes sense for amici, so the base method would not have those, but the rest should work? |
Exactly those. Breaks encapsulation and adds a cyclic dependency. |
Yes that's what I noticed (cyclic dependency) as well. |
P.S.: I'd rather remove the AMICI part. In my opinion, these settings should have been applied consciously by the user before running any checks here. |
So remove the |
This suggestion resolves this right? |
Removed the Amici dependencies, still have to move functionality to |
Functionality now moved to BaseObjective, added delegate to |
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.
looks good to me, thanks! feel free to merge when you think it's fine.
|
||
Returns | ||
------- | ||
bool |
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.
agreed, call it "match" or something
objective.amici_solver.setAbsoluteTolerance(1e-10) | ||
objective.amici_solver.setRelativeTolerance(1e-12) | ||
|
||
self.assertFalse(petab_problem.check_gradients( |
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.
why is this False again? maybe add a scenario where it's true? this https://github.com/ICB-DCM/pyPESTO/blob/develop/test/util.py#L122 should be stable enough no matter what.
Co-authored-by: Yannik Schälte <31767307+yannikschaelte@users.noreply.github.com>
Check for the gradients of the model equations and returns
False
if gradients are incompatible / not well-suited for gradient-based optimizers otherwise returnsTrue
.