Consolidate different gradient checks

[jvanhoefer]

Feature description Currently we have several different (rather redundant) gradient checking routines, which only have slightly different functionalities, which in my opinion do not all justify being there in parallel. (In the end: We want to have a minimal interface to the outside world to be user-friendly):

In objective/base https://github.com/ICB-DCM/pyPESTO/blob/0aaa34179497c429434b4b6d2aeb35309b113961/pypesto/objective/base.py#L370

https://github.com/ICB-DCM/pyPESTO/blob/0aaa34179497c429434b4b6d2aeb35309b113961/pypesto/objective/base.py#L422

https://github.com/ICB-DCM/pyPESTO/blob/0aaa34179497c429434b4b6d2aeb35309b113961/pypesto/objective/base.py#L589

In the PEtab importer https://github.com/ICB-DCM/pyPESTO/blob/fb2be819b41411dc1686d8429ce5efa6c535b70b/pypesto/petab/importer.py#L83

Motivation/Application I think, that having one FD check in objective/base and maybe another one for checking multiple eps values is great. The one in the PEtab importer is clearly redundant, since you would rather create a problem object, and then call a gradient check on that (which is also, what the function does...). The third function in the objective base also seems close to the other ones and not "general purpose".

Therefore I would like to

a) propose to get rid of the gradient check in the PEtab importer and would like to

b) open the discussion, if the third gradient check in objective/base adds enough "general purpose" value, that it justifies to have and maintain an other gradient checking method there.

Opinions? @yannikschaelte @dweindl @stephanmg

[stephanmg]

Dear @jvanhoefer,

great idea!

As we already discussed last week I think, the method check_gradients_match_finite_differences will be used only internally, so on my pyPESTO fork I made changes accordingly by prefixing the method with _. I think this doesn't have to be part of pyPESTO's public API.

Instead, I've added a visualization method which uses _check_gradients_match_finite_differences to visualize gradients and might be more useful to the general public. I will request a PR / review once I am fully satisfied with the implementation of my gradient consistency visualization plotting routines on my branch to integrate this eventually into pyPESTO.

The check_gradients method in the PEtab importer would be still useful as it checks for consistency differently. Perhaps a simple renaming would clear things up?

[yannikschaelte]

If I see correctly, (1)+(2) should be the base implementation. (4) autofills petab nominal parameters, which seems reasonable (not sure if there is room for simplification there though). (3) conceptually takes the output of (2) and actually compares the obtained atols+rtols, which (1)+(2) fail to do. Thus I guess (3) also makes sense to keep, as otherwise one would need to have binary output for (1)+(2) whether the df or bool is of interest. It seems however that (3) could be simplified to e.g. not redefine mode + multi_eps, but just call (2) and evalute the outputs.

[jvanhoefer]

Regarding (4): What about having one of (1)-(3) an optional "PEtab importer" argument, which would allow to get the nominal parameters from there? As the current implementation does require the problem to be created internally (i.e. potentially the model to be compiled). Hence calling the gradient check on the model instance seems more reasonable to me...?

[dweindl]

Didn't have a closer a closer look at the code, but @yannikschaelte's comment sounds reasonable.

Regarding (4): What about having one of (1)-(3) an optional "PEtab importer" argument, which would allow to get the nominal parameters from there? As the current implementation does require the problem to be created internally (i.e. potentially the model to be compiled). Hence calling the gradient check on the model instance seems more reasonable to me...?

Strongly opposed. Introducing a PEtab+AMICI dependency in pypesto/objective/base.py completely breaks the modularity we so far tried to maintain.

[yannikschaelte]

Didn't have a closer a closer look at the code, but @yannikschaelte's comment sounds reasonable.

Regarding (4): What about having one of (1)-(3) an optional "PEtab importer" argument, which would allow to get the nominal parameters from there? As the current implementation does require the problem to be created internally (i.e. potentially the model to be compiled). Hence calling the gradient check on the model instance seems more reasonable to me...?

Strongly opposed. Introducing a PEtab+AMICI dependency in pypesto/objective/base.py completely breaks the modularity we so far tried to maintain.

One could use fancy oo design with an abstract "GiveMeAParameterBob" class, allowing to either pass a ndarray or said object, with instance checking, which the PetabImporter could implement, but sounds a bit over the top.

[jvanhoefer]

Strongly opposed. Introducing a PEtab+AMICI dependency in pypesto/objective/base.py completely breaks the modularity we so far tried to maintain.

Agree, that was also not, how I meant my suggestion. :)

[stephanmg]

As far as this concerns the function I've added, I can consolidate in the following way:

1. check_gradients function in PEtab importer can be removed from the code base (The function can be implemented outside of pyPESTO if really nobody thinks it might be useful to have)

2. check_gradients_match_finite_differences can be made pseudo-private to not clutter the public API. It willl be used only internally in a visualization method I plan to PR into pyPESTO anyway.

Does this sound reasonable? Feel free to assign me if you think that's appropriate.

[dweindl]

And one more finite differences implementation to get rid of: https://github.com/ICB-DCM/pyPESTO/blob/f9d68efc962f10dbe8d5715dd782defce2c96924/doc/example/petab_import.ipynb