Basic finite differences #666

yannikschaelte · 2021-05-13T17:07:56Z

Implementation of central finite differences as an objective class. Can be wrapped around existing objectives to provide finite difference approximations to derivatives (grad, hess, sres).

Not implemented yet:

~~Choice of method (forward, backward, centered=only),~~
step length adaption,
efficient handling of fixed parameters.
one could save a few evaluations here and there, but as those are only O(n_par), I currently don't care ...

closes #18

…o feature_result_from_history

…DCM/pyPESTO into feature_result_from_history

yannikschaelte · 2021-05-13T17:08:53Z

~~waiting for #663~~

codecov-commenter · 2021-05-13T22:03:36Z

Codecov Report

Merging #666 (d7fb77e) into develop (1b63597) will decrease coverage by 52.87%.
The diff coverage is 23.17%.

@@             Coverage Diff              @@
##           develop     #666       +/-   ##
============================================
- Coverage    87.71%   34.83%   -52.88%     
============================================
  Files           95       96        +1     
  Lines         6136     6350      +214     
============================================
- Hits          5382     2212     -3170     
- Misses         754     4138     +3384

Impacted Files	Coverage Δ
pypesto/__init__.py	`100.00% <ø> (ø)`
pypesto/objective/history.py	`47.68% <ø> (-47.69%)`	⬇️
pypesto/optimize/__init__.py	`100.00% <ø> (ø)`
pypesto/objective/finite_difference.py	`15.00% <15.00%> (ø)`
pypesto/objective/base.py	`53.14% <85.71%> (-34.58%)`	⬇️
pypesto/objective/__init__.py	`100.00% <100.00%> (ø)`
pypesto/objective/aggregated.py	`25.49% <100.00%> (-74.51%)`	⬇️
pypesto/objective/function.py	`82.65% <100.00%> (-5.31%)`	⬇️
pypesto/optimize/optimizer.py	`82.71% <100.00%> (-7.40%)`	⬇️
pypesto/objective/aesara.py	`0.00% <0.00%> (-92.93%)`	⬇️
... and 70 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b63597...d7fb77e. Read the comment docs.

pypesto/objective/aggregated.py

pypesto/optimize/optimizer.py

pypesto/objective/function.py

Co-authored-by: Paul Jonas Jost <70631928+PaulJonasJost@users.noreply.github.com>

jvanhoefer · 2021-05-17T08:20:27Z

I currently don't see an example, might be nice to have something like that? :)

yannikschaelte · 2021-05-17T09:00:16Z

I currently don't see an example, might be nice to have something like that? :)

One could make one, but I think that would be rather artificial at the moment and refactored anyway when we decide on how to structure the pyPESTO tutorial, therefore I would rather wait until then with an example?

…xes_ys

paulstapor

Approved, looks good

pypesto/objective/aggregated.py

paulstapor · 2021-05-17T08:44:02Z

test/base/test_objective.py

+    return request.param
+
+
+def test_fds(fd_method):


Nice test, does exactly what it should!

pypesto/objective/finite_difference.py

paulstapor · 2021-05-17T08:52:19Z

pypesto/objective/finite_difference.py

+    delta_fun:
+        FD step sizes for gradient and Hessian.
+        Can be either a float, or a :class:`np.ndarray` of shape (n_par,)
+        for different step sizes for different coordinates.
+    delta_res:
+        FD step sizes for residual sensitivities.
+        Similar to `delta_fun`.


Why do we have a distinct step size for sres, but not for the Hessian? If we already open the box of allowing different step sizes for different derivatives: Then also provide one for "hessian via gradients"... right?

agreed, adding one

paulstapor · 2021-05-17T08:53:22Z

pypesto/objective/finite_difference.py

+        FD step sizes for residual sensitivities.
+        Similar to `delta_fun`.
+    method:
+        Method to calculate FDs. Currently, only "center" is supported.


Overall for good reasons... Would even omit this option totally. Central FD are typically (and from theory) much more accurate... I don't see a good reason for only allowing forward or backward ones...

pypesto/objective/finite_difference.py

paulstapor · 2021-05-17T10:38:18Z

pypesto/objective/finite_difference.py

+                hess[ix1, ix2] = hess[ix2, ix1] = \
+                    (fpp - fpm - fmp + fmm) / (delta1_val * delta2_val)


There's also a particular form of 2nd order FD for the diagonal entries of the Hessian, which should be however equivalent to this here... Probably, the particular form is just a simplification, which comes out after plugging in the values from this general form here. However, Might make sense to double check. At least, I would say this here is correct...

note the diagonal is computed a few lines above

ah, indeed... Sorry, seems like I missed that one...

paulstapor · 2021-05-17T10:39:35Z

I currently don't see an example, might be nice to have something like that? :)

One could make one, but I think that would be rather artificial at the moment and refactored anyway when we decide on how to structure the pyPESTO tutorial, therefore I would rather wait until then with an example?

Would also agree to add an example. However, also fine with adding one in the tutorial. In this case, please open an issue that this won't be forgotten! :)

yannikschaelte · 2021-05-17T11:37:32Z

I currently don't see an example, might be nice to have something like that? :)

One could make one, but I think that would be rather artificial at the moment and refactored anyway when we decide on how to structure the pyPESTO tutorial, therefore I would rather wait until then with an example?

Would also agree to add an example. However, also fine with adding one in the tutorial. In this case, please open an issue that this won't be forgotten! :)

Added an example in the docstring now at least ...

jvanhoefer

Looks fine, only minor comments :) (I am a bit curious about the "inconsistency" in the hessian between mixed and non-mixed 2nd derivatives...)

pypesto/objective/base.py

jvanhoefer · 2021-05-17T08:28:54Z

pypesto/objective/base.py

+                or 1 in sensi_orders and not self.has_sres
+            )


Suggested change

or 1 in sensi_orders and not self.has_sres

)

or 1 in sensi_orders and not self.has_sres

or 2 in sensi_orders

)

What if Hessian information is asked for in MODE_RES? (Even though current residual-based optimizers might not need that info)

not aware of any optimizer that does as most stick to the linear structure ... but could of course be added then.

jvanhoefer · 2021-05-17T08:40:22Z

pypesto/objective/finite_difference.py

+
+        if isinstance(delta_fun, str) or isinstance(delta_res, str):
+            raise NotImplementedError(
+                "Adaptive FD step sizes are not implemented yet.",


I don't know, why it is getting sexy recently having a comma in the end of anything recently (My taste remains old-school there... :D) but if we do that, then I would do it consistently (e.g. two lines below as well...)

I mean in f"Method must be one of {methods}"

haha it is recommended because it allows modifying less lines when e.g. adding additional arguments. true, adding one below.

jvanhoefer · 2021-05-17T09:25:01Z

pypesto/objective/finite_difference.py

+        Derivative method for the gradient.
+    hess:
+        Derivative method for the Hessian
+    sres:
+        Derivative method for the residual sensitivities.


tbh I do not completely get from that, what e.g. grad=True or grad=None would mean. Where would I get the gradient from in these cases? The objective or FD? And also, why is the Derivative method a boolean value? :D

{True, False, None} here is a ternary variable: None means whatever is available, True means always FD, False never. Documented that a few lines above, will add hints.

jvanhoefer · 2021-05-17T09:25:42Z

pypesto/objective/finite_difference.py

+        # This is the main method to overwrite from the base class, it handles
+        #  and delegates the actual objective evaluation.


-> doc string?

would rather not use docstring here, as the docstring should be the one from the base class, this here is just a hint to the programmer.

jvanhoefer · 2021-05-17T11:54:47Z

pypesto/objective/finite_difference.py

+            elif self.method == FD.FORWARD:
+                f2p = f_fval(x + 2 * delta1)
+                fc = f_fval(x + delta1)
+                f2m = fval
+            elif self.method == FD.BACKWARD:
+                f2p = fval
+                fc = f_fval(x - delta1)
+                f2m = f_fval(x - 2 * delta1)


I am not so sure about that, doesn't that essentially approximate the 2nd derivative at x+delta? But I guess that is what the forward/backward mode does for the 1st derivative as well, so should be fine...

it is (f'(x) - f'(x-d)) / d \approx ( (f(x) - f(x-d)) / d - (f(x-d) - f(x-2d)) / d) / d, so yes should be correct.

jvanhoefer · 2021-05-17T12:06:12Z

pypesto/objective/finite_difference.py

+                if self.method == FD.CENTRAL:
+                    fpp = f_fval(x + delta1 / 2 + delta2 / 2)
+                    fpm = f_fval(x + delta1 / 2 - delta2 / 2)
+                    fmp = f_fval(x - delta1 / 2 + delta2 / 2)
+                    fmm = f_fval(x - delta1 / 2 - delta2 / 2)
+                elif self.method == FD.FORWARD:
+                    fpp = f_fval(x + delta1 + delta2)
+                    fpm = f_fval(x + delta1 + 0)
+                    fmp = f_fval(x + 0 + delta2)
+                    fmm = fval
+                elif self.method == FD.BACKWARD:
+                    fpp = fval
+                    fpm = f_fval(x + 0 - delta2)
+                    fmp = f_fval(x - delta1 + 0)
+                    fmm = f_fval(x - delta1 - delta2)


I mean, there is the possibility to take the f_fval(x+delta1 + delta2) here and elsewhere (so drop the /2 everywhere) and instead go for (fpp - fpm - fmp + fmm) / (4 * delta1_val * delta2_val). This would be consistent with the diagonal elements...?

Even though now I am confused, why there is no 4* in the diagonals...?

the step size should always be delta, i.e. from x - delta/2 to x + delta/2 (central), or from x to x + delta (forward). I think that should be consistent ...

I think in the matlab implementation, it used a stepsize of delta for the off-diagonals, and delta/2 on the diagonals, which may be not the most consistent ... even though one would save a few evaluations when simultaneously computing gradients.

pypesto/objective/finite_difference.py

…xes_ys

PaulJonasJost · 2021-05-17T13:31:45Z

pypesto/objective/base.py

-            Boolean indicating whether mode is supported
-        """
-        raise NotImplementedError()
+        if (


I was wondering about that in another PR but shouldn't we implement something that checks that the sensi_orders are even valid? right now if we would call check_sensi_orders(sensi_orders=(3,)) it would return True. How about adding an if(max(sensi_orders)>2): raise ValueError?

would make sense

PaulJonasJost and others added 20 commits May 12, 2021 12:39

added option for hdf5 History to read_result_from_file

21af5a8

Merge branch 'develop' into feature_result_from_history

0373a21

updated get_value_trace of Hdf5 History

3ff76e5

Merge remote-tracking branch 'origin/feature_result_from_history' int…

87b3ed7

…o feature_result_from_history

updated test to include hdf5 history.

55e04bd

Merge branch 'develop' into feature_result_from_history

6f8a005

updated test to include hdf5 history.

fcfb366

Merge branch 'feature_result_from_history' of https://github.com/ICB-…

f12263b

…DCM/pyPESTO into feature_result_from_history

Update pypesto/optimize/optimizer.py

c19ce02

updated test

c16dbd5

Merge branch 'feature_result_from_history' of https://github.com/ICB-…

8c9b0d5

…DCM/pyPESTO into feature_result_from_history

Merge branch 'develop' into feature_result_from_history

58568ce

updated test_history

34150a8

updated check_reconstruc_history

568c7a9

Update test/base/test_history.py

727abd0

Update pypesto/objective/history.py

f03e222

Merge branch 'develop' into feature_result_from_history

7db97d5

Update pypesto/objective/history.py

1c933b4

updated check_for_not_nan_entries

2e9db10

add finite difference implementation

04a67b7

tidy up

8fa1c89

yannikschaelte mentioned this pull request May 13, 2021

Approximate grad and hess #18

Closed

yannikschaelte added 2 commits May 13, 2021 19:18

fix docs

f6db1ac

add test for grad based hess

cfc15e2

yannikschaelte requested review from jvanhoefer and paulstapor May 13, 2021 20:01

yannikschaelte self-assigned this May 13, 2021

yannikschaelte added 2 commits May 13, 2021 23:20

refactor

e1c9b6c

fix typo

967c7b5

yannikschaelte added 3 commits May 14, 2021 14:54

implement forward, backward

cc3a7df

Merge branch 'develop' into fixes_ys

5493a26

fix docs

5297839

yannikschaelte requested a review from PaulJonasJost May 14, 2021 13:08

yannikschaelte added 2 commits May 14, 2021 15:14

use readable variables

a3a5c80

remove superfluous method

5450465

PaulJonasJost approved these changes May 14, 2021

View reviewed changes

pypesto/objective/aggregated.py Outdated Show resolved Hide resolved

pypesto/optimize/optimizer.py Outdated Show resolved Hide resolved

pypesto/objective/function.py Show resolved Hide resolved

yannikschaelte and others added 2 commits May 14, 2021 23:00

Update pypesto/objective/aggregated.py

4b6822b

Co-authored-by: Paul Jonas Jost <70631928+PaulJonasJost@users.noreply.github.com>

Update pypesto/optimize/optimizer.py

cdfe2a4

Co-authored-by: Paul Jonas Jost <70631928+PaulJonasJost@users.noreply.github.com>

yannikschaelte added 4 commits May 17, 2021 11:16

fix docs

273dabe

Merge branch 'fixes_ys' of https://github.com/icb-dcm/pypesto into fi…

2be9aae

…xes_ys

add example

c29b0f6

make nice

65764a1

paulstapor approved these changes May 17, 2021

View reviewed changes

review comments

8104310

Merge branch 'develop' into fixes_ys

6d58063

jvanhoefer approved these changes May 17, 2021

View reviewed changes

PaulJonasJost and others added 3 commits May 17, 2021 15:14

Merge branch 'develop' into fixes_ys

d0ebd16

review comments

8b89c9b

Merge branch 'fixes_ys' of https://github.com/icb-dcm/pypesto into fi…

d7fb77e

…xes_ys

PaulJonasJost reviewed May 17, 2021

View reviewed changes

yannikschaelte merged commit 0e61e0f into develop May 17, 2021

yannikschaelte deleted the fixes_ys branch May 17, 2021 13:40

yannikschaelte mentioned this pull request May 17, 2021

Release 0.2.6 #655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic finite differences #666

Basic finite differences #666

yannikschaelte commented May 13, 2021 •

edited

Loading

yannikschaelte commented May 13, 2021 •

edited

Loading

codecov-commenter commented May 13, 2021 •

edited

Loading

jvanhoefer commented May 17, 2021

yannikschaelte commented May 17, 2021

paulstapor left a comment

paulstapor May 17, 2021

paulstapor May 17, 2021

yannikschaelte May 17, 2021

paulstapor May 17, 2021

paulstapor May 17, 2021

yannikschaelte May 17, 2021

paulstapor May 17, 2021

paulstapor commented May 17, 2021

yannikschaelte commented May 17, 2021

jvanhoefer left a comment

jvanhoefer May 17, 2021

jvanhoefer May 17, 2021

yannikschaelte May 17, 2021

jvanhoefer May 17, 2021

jvanhoefer May 17, 2021

yannikschaelte May 17, 2021

jvanhoefer May 17, 2021

yannikschaelte May 17, 2021

jvanhoefer May 17, 2021

yannikschaelte May 17, 2021

jvanhoefer May 17, 2021

yannikschaelte May 17, 2021

yannikschaelte May 17, 2021

jvanhoefer May 17, 2021

jvanhoefer May 17, 2021

yannikschaelte May 17, 2021

yannikschaelte May 17, 2021

PaulJonasJost May 17, 2021

yannikschaelte May 17, 2021

		hess[ix1, ix2] = hess[ix2, ix1] = \
		(fpp - fpm - fmp + fmm) / (delta1_val * delta2_val)

		# This is the main method to overwrite from the base class, it handles
		# and delegates the actual objective evaluation.

Basic finite differences #666

Basic finite differences #666

Conversation

yannikschaelte commented May 13, 2021 • edited Loading

yannikschaelte commented May 13, 2021 • edited Loading

codecov-commenter commented May 13, 2021 • edited Loading

Codecov Report

jvanhoefer commented May 17, 2021

yannikschaelte commented May 17, 2021

paulstapor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulstapor commented May 17, 2021

yannikschaelte commented May 17, 2021

jvanhoefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannikschaelte commented May 13, 2021 •

edited

Loading

yannikschaelte commented May 13, 2021 •

edited

Loading

codecov-commenter commented May 13, 2021 •

edited

Loading