Skip to content
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

Hybrid grad check #27

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8f590f5
Adding HybridDerivativeCheck class.
stephanmg May 5, 2023
85e4932
Reformatting with black.
stephanmg May 5, 2023
30cbd19
Reverting existing test case to prior state.
stephanmg May 5, 2023
21ba0f2
Adding rtol and atol.
stephanmg May 5, 2023
71449f4
Fix.
stephanmg May 5, 2023
b384821
Merged main into hybrid_grad_check.
stephanmg May 5, 2023
481401d
Merge branch 'ICB-DCM:main' into hybrid_grad_check
stephanmg May 9, 2023
45acfb9
Merged and adding test case.
stephanmg May 10, 2023
e75f852
Formatting.
stephanmg May 10, 2023
ee666db
Clarified comment on new hybrid derivative check test case.
stephanmg May 10, 2023
fd106c5
Merge remote-tracking branch 'upstream/main' into hybrid_grad_check
stephanmg May 10, 2023
20e8a1d
Update fiddy/derivative_check.py
stephanmg Jun 30, 2023
c6e0d9a
Update fiddy/derivative_check.py
stephanmg Jun 30, 2023
52827e6
Update fiddy/derivative_check.py
stephanmg Jun 30, 2023
8fad9b7
Addressing review comments.
stephanmg Oct 5, 2023
f2d8c23
Merged.
stephanmg Oct 5, 2023
6cacf43
Merged.
stephanmg Oct 5, 2023
9bc3d18
Fix RTD.
stephanmg Nov 6, 2023
beb2fb9
Update fiddy/derivative_check.py
dilpath Nov 7, 2023
9d9747a
use `get_expected_and_test_values` in `NumpyIsCloseDerivativeCheck`
dilpath Nov 7, 2023
51279b5
Update fiddy/derivative_check.py
stephanmg Nov 13, 2023
6b6c327
Addressing DP's review.
stephanmg Nov 13, 2023
e0789e3
Merge branch 'hybrid_grad_check' of github.com:stephanmg/fiddy into h…
stephanmg Nov 13, 2023
75e4cf9
Fix indent.
stephanmg Nov 13, 2023
0922139
Fix check?
stephanmg Nov 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 108 additions & 1 deletion fiddy/derivative_check.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import abc
from typing import Any, Callable, Dict, List, Union
from itertools import product
from itertools import chain

from dataclasses import dataclass

import numpy as np
import pandas as pd
import math

from .constants import (
# TYPE_DIMENSION,
Expand Down Expand Up @@ -161,3 +162,109 @@ def method(self, *args, **kwargs):
success=success,
)
return derivative_check_result


class HybridDerivativeCheck(DerivativeCheck):
method_id = "hybrid"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you doc the logic of this class in a class docstring here? i.e. the formula/algorithm used to determine whether gradients are correct. Will be great for when I write the remaining docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. LMK if more is needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I meant the actual formula, preferably with latex. There's an example of latex in RTD for amici.import_utils.noise_distribution_to_cost_function here [1], with the docstring here [2].

But this can also wait until you publish the formula. If you decide to wait, could you add a TODO to do this later? Would be great to add a reference to your paper when it's published, too.

[1] https://amici.readthedocs.io/en/latest/generated/amici.import_utils.html#amici.import_utils.noise_distribution_to_cost_function
[2] https://github.com/AMICI-dev/AMICI/blob/3c5e997df3655c26dde35705ef25b2a0f419fe8b/python/sdist/amici/import_utils.py#L105-L107


def method(self, *args, **kwargs):
expected_values = []
test_values = []
success = True
for direction_index, directional_derivative in enumerate(
self.derivative.directional_derivatives
):
test_value = directional_derivative.value
test_values.append(test_value)

expected_value = []
for output_index in np.ndindex(self.output_indices):
element = self.expectation[output_index][direction_index]
expected_value.append(element)
expected_value = np.array(expected_value).reshape(test_value.shape)
expected_values.append(expected_value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the same in NumpyIsCloseDerivativeCheck, could move it to its own function, or DerivativeCheck.__call__. Not important here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored: ExtractMethod.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I implemented the refactor in NumpyIsCloseDerivativeCheck now too.


# debug
assert len(expected_values) == len(
test_values
), "Mismatch of step sizes"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove, or raise some specific exception? This is always due to a user error, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should always be a user error, removed.


results_all = []
directional_derivative_check_results = []
for step_size in range(0, len(expected_values)):
approxs_for_param = []
grads_for_param = []
Comment on lines +189 to +190
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be reset in the inner loop, instead of here in the outer loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be okay as-is.

results = []
for diff_index, directional_derivative in enumerate(
self.derivative.directional_derivatives
):
try:
for grad, approx in zip(
expected_values[diff_index - 1][step_size - 1],
test_values[diff_index - 1][step_size - 1],
):
approxs_for_param.append(approx)
grads_for_param.append(grad)
fd_range = np.percentile(approxs_for_param, [0, 100])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just the min and max of approxs_for_param?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

fd_mean = np.mean(approxs_for_param)
grad_mean = np.mean(grads_for_param)
if not (fd_range[0] <= grad_mean <= fd_range[1]):
if np.any(
[
abs(x - y) > kwargs["atol"]
for i, x in enumerate(approxs_for_param)
for j, y in enumerate(approxs_for_param)
if i != j
]
):
fd_range = abs(fd_range[1] - fd_range[0])
if (
abs(grad_mean - fd_mean)
/ abs(fd_range + np.finfo(float).eps)
) > kwargs["rtol"]:
results.append(False)
else:
results.append(False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handling of results needs some work. For example, here both cases of the if-else have results.append(False).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

else:
results.append(
None
) # can't judge consistency / questionable grad approxs
else:
fd_range = abs(fd_range[1] - fd_range[0])
if math.isinf(
(fd_range)
or math.isnan(fd_range)
or math.isinf(fd_mean)
or math.isnan(fd_mean)
):
stephanmg marked this conversation as resolved.
Show resolved Hide resolved
results.append(None)
else:
results.append(True)
stephanmg marked this conversation as resolved.
Show resolved Hide resolved
except (IndexError, TypeError):
# TODO: Fix this, why does this occur?
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a reproducible example of this? I could take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now.


directional_derivative_check_result = (
DirectionalDerivativeCheckResult(
direction_id=directional_derivative.id,
method_id=self.method_id,
test=test_value,
expectation=expected_value,
output={"return": results},
success=all(results),
)
)
directional_derivative_check_results.append(
directional_derivative_check_result
)
results_all.append(results)

success = all(chain(*results_all))
derivative_check_result = DerivativeCheckResult(
method_id=self.method_id,
directional_derivative_check_results=directional_derivative_check_results,
test=self.derivative.value,
expectation=self.expectation,
success=success,
)
return derivative_check_result
4 changes: 2 additions & 2 deletions fiddy/gradient_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def gradient_check(

if sort:
results_df.sort_values(
by=['dimension', 'size'],
by=["dimension", "size"],
inplace=True,
)

Expand Down Expand Up @@ -266,7 +266,7 @@ def keep_lowest_error(
if not inplace:
sort_df = minimal_results_df
sort_df.sort_values(
['success', 'dimension', 'size'],
["success", "dimension", "size"],
ascending=[False, True, True],
inplace=True,
)
Expand Down
21 changes: 12 additions & 9 deletions fiddy/success.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ def method(
success_by_size[size] = np.isclose(
values,
np.nanmean(values, axis=0),
rtol=self.rtol/2,
atol=self.atol/2,
rtol=self.rtol / 2,
atol=self.atol / 2,
equal_nan=self.equal_nan,
).all()

Expand All @@ -108,13 +108,16 @@ def method(
success = False
value = np.nanmean(np.array(consistent_results), axis=0)
if consistent_results:
success = np.isclose(
consistent_results,
value,
rtol=self.rtol,
atol=self.atol,
equal_nan=self.equal_nan
).all() and not np.isnan(consistent_results).all()
success = (
np.isclose(
consistent_results,
value,
rtol=self.rtol,
atol=self.atol,
equal_nan=self.equal_nan,
).all()
and not np.isnan(consistent_results).all()
)
value = np.average(np.array(consistent_results), axis=0)

return success, value
57 changes: 54 additions & 3 deletions tests/test_derivative.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
from fiddy.derivative import Computer
from fiddy.analysis import ApproximateCentral
from fiddy.success import Consistency
from fiddy.derivative_check import NumpyIsCloseDerivativeCheck
from fiddy.derivative_check import (
NumpyIsCloseDerivativeCheck,
HybridDerivativeCheck,
)


RTOL = 1e-2
Expand Down Expand Up @@ -108,7 +111,7 @@ def test_get_derivative(point, sizes, output_shape):
# FIXME default?
sizes=[1e-10, 1e-5],
# FIXME default?
method_ids=[MethodId.FORWARD, MethodId.BACKWARD],
method_ids=[MethodId.FORWARD, MethodId.BACKWARD, MethodId.CENTRAL],
# FIXME default?
analysis_classes=[ApproximateCentral],
# FIXME default? not just "True" ...
Expand All @@ -122,7 +125,55 @@ def test_get_derivative(point, sizes, output_shape):
expectation=expected_value,
point=point,
)
result = check(rtol=1e-2)
result = check(rtol=1e-2, atol=1e-3)
assert result.success


@pytest.mark.parametrize(
"point, sizes, output_shape",
[
(np.array(point), sizes, output_shape)
for point in [
(1, 0, 0),
(0.9, 0.1, 0.2, 0.4),
]
for sizes in [
[1e-10, 1e-5],
]
for output_shape in [
(1,),
(1, 2),
(5, 3, 6, 2, 4),
]
],
)
def test_get_derivative_hybrid(point, sizes, output_shape):
function = partial(rosenbrock, output_shape=output_shape)
expected_derivative_function = partial(
rosenbrock_der, output_shape=output_shape
)
derivative = get_derivative(
function=function,
point=point,
# FIXME default?
sizes=[1e-10, 1e-5],
# FIXME default?
method_ids=[MethodId.FORWARD, MethodId.BACKWARD, MethodId.CENTRAL],
# FIXME default?
analysis_classes=[ApproximateCentral],
# FIXME default? not just "True" ...
success_checker=Consistency(),
)
test_value = derivative.value
expected_value = expected_derivative_function(point)

check = HybridDerivativeCheck(
derivative=derivative,
expectation=expected_value,
point=point,
)
stephanmg marked this conversation as resolved.
Show resolved Hide resolved
# based on given tolerances, hybrid gradient check should not fail
result = check(rtol=1e-2, atol=1e-3)
assert result.success


Expand Down