Hybrid grad check #27
Conversation
|
Added test case now. See TODO in |
fiddy/derivative_check.py
Outdated
| 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) |
There was a problem hiding this comment.
Since this is the same in NumpyIsCloseDerivativeCheck, could move it to its own function, or DerivativeCheck.__call__. Not important here.
There was a problem hiding this comment.
Refactored: ExtractMethod.
There was a problem hiding this comment.
Thanks, I implemented the refactor in NumpyIsCloseDerivativeCheck now too.
fiddy/derivative_check.py
Outdated
| # debug | ||
| assert len(expected_values) == len( | ||
| test_values | ||
| ), "Mismatch of step sizes" |
There was a problem hiding this comment.
Remove, or raise some specific exception? This is always due to a user error, right?
There was a problem hiding this comment.
Should always be a user error, removed.
fiddy/derivative_check.py
Outdated
| class HybridDerivativeCheck(DerivativeCheck): | ||
| method_id = "hybrid" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Added. LMK if more is needed.
There was a problem hiding this comment.
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
fiddy/derivative_check.py
Outdated
| except (IndexError, TypeError): | ||
| # TODO: Fix this, why does this occur? | ||
| pass |
There was a problem hiding this comment.
Do you have a reproducible example of this? I could take a look
There was a problem hiding this comment.
Should be fixed now.
Co-authored-by: Dilan Pathirana <[email protected]>
Co-authored-by: Dilan Pathirana <[email protected]>
Co-authored-by: Dilan Pathirana <[email protected]>
stephanmg
force-pushed
the
hybrid_grad_check
branch
from
aaf3bf2 to
9bc3d18
Compare
|
PR doesn't update... |
| approxs_for_param = [] | ||
| grads_for_param = [] |
There was a problem hiding this comment.
Should this be reset in the inner loop, instead of here in the outer loop?
There was a problem hiding this comment.
Should be okay as-is.
| ): | ||
| approxs_for_param.append(approx) | ||
| grads_for_param.append(grad) | ||
| fd_range = np.percentile(approxs_for_param, [0, 100]) |
There was a problem hiding this comment.
Is this just the min and max of approxs_for_param?
fiddy/derivative_check.py
Outdated
| if ( | ||
| abs(grad_mean - fd_mean) | ||
| / abs(fd_range + np.finfo(float).eps) | ||
| ) > kwargs["rtol"]: | ||
| results.append(False) | ||
| else: | ||
| results.append(False) |
There was a problem hiding this comment.
The handling of results needs some work. For example, here both cases of the if-else have results.append(False).
| if not np.isfinite([fd_range, fd_mean]).all(): | ||
| results.append(None) | ||
| else: | ||
| result = True | ||
| results.append(result) |
There was a problem hiding this comment.
Need to do result = ... everywhere (if going with this coding style), and finally results.append(result) at the end (outside of this else statement). I can do this, will wait until the other comments are resolved.
Co-authored-by: Dilan Pathirana <[email protected]>
No description provided.