Implementation of a Parametrised Reduced Functional - #241
Conversation
…educedFunctional` where parameters can be updated but are not included in the derivative calculations: 1. Adds a `parameter_update` method 2. Parameters are appended at the end of the list of optimization controls, so `derivative_components` is not a required argument. 3. The `derivative` method returns only derivative corresponding to optimization controls.
|
In the current implementation prf = ParameterisedReducedFunctional(functional, user_controls, parameters)
assert len(prf.controls) == len(user_controls) + len(parameters)This will also be a problem later because the optimisers will expect: len(prf.derivative()) == len(prf.controls)when you will actually have (correctly): len(prf.derivative()) == len(prf.user_controls)If this is the case then you may need to override the To get around this, you may have to instead inherit from the |
…e abstract base class
|
In this intermediate implementation, the However, a lot of the code is duplicated, and, as @colinjcotter suggested, a more efficient way to do this would be to call |
…dFunctional` internally: - Instead of inheriting the `AbstractReducedFunctional` or `ReducedFunctional` classes, `ParametrisedReducedFunctional` simply calls a `ReducedFunctional` object internally and passes the controls and parameters together as `all_controls`
`ParametrisedReducedFunctional` must be a subclass of `AbstractReducedFunctional` Co-authored-by: Josh Hope-Collins <jhc.jss@gmail.com>
…h component of the parameter list must first be wrapped in `Control`.
1. Basic test to check `call`, `derivative` and `parameter_update` methods 2. Combination tests with single/multiple controls and single/multiple parameters 3. Tests to check behaviour of `controls` and `parameters` property 4. Evaluation on a more complex example 5. Tests to check behaviour in case of multiple parameter updates before call.
…cedFunctional` with `derivative_components`.
|
Considering the discussion at the Firedrake meeting, the following commits rewrite |
1. `ParametrisedReducedFunctional` has been removed in favour of a `ReducedFunctional` that accepts `parameters` as an argument, along with a check to make sure either `derivative_components` or `parameters` is passed, but not both simultaneously. 2. If `parameters` is passed, a new `ReducedFunctional` object is created recursively. Methods will check if the `parameters` attribute is present to branch out their implementation. 3. Derivative callback include a `parameters` argument in their signature.
…est to validate initialization of `ReducedFunctional` with either `derivative_components` or `parameters`
|
@JHopeCollins, with respect to the recent CI failure: @no_annotations
def derivative(self, adj_input=1.0, apply_riesz=False):
values = [c.tape_value() for c in self.controls]
> controls = self.derivative_cb_pre(self.parameters if hasattr(self, "_parameters") else None, self.controls)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E TypeError: EnsembleReducedFunctional.<lambda>() takes 1 positional argument but 2 were givenThis happens because the derivative callback now has the signature |
If the user has not passed parameters then we shouldn't change the callback interface otherwise we will break current code before the deprecation cycle is done. For now the callback signature will depend on whether parameters are passed or not. |
47ef338 to
6f74a9f
Compare
1. This commit introduces the functions `_call_derivative_cb_pre` and `_call_derivative_cb_post` to handle callbacks if `parameters` are passed, or maintain backwards compatibility and a deprecation warning if only `controls` are passed.
@JHopeCollins, I've added the functions |
| else: | ||
| full_values = values + self._parameters | ||
| return self._reduced_functional(full_values) |
There was a problem hiding this comment.
This case will cause the tape to be evaluated twice. Put this check at the beginning of the method so we recurse early before any recomputations.
| if not hasattr(self, "_parameters"): | ||
| return tlm | ||
| else: | ||
| # self._reduced_functional.tlm will expect len(m_dot) = len(self._all_controls), so we pad it with zeros. | ||
| m_dot_all = Enlist(m_dot) + [p._ad_init_zero() for p in self._parameters] | ||
| tlm_all = self._reduced_functional.tlm(m_dot_all) | ||
| return tlm_all |
There was a problem hiding this comment.
Same as __call__, move this check to the beginning of the method.
| except TypeError: | ||
| warnings.warn( | ||
| message="derivative_cb_pre should accept (controls, parameters)." | ||
| "Falling back to deprecated signature (controls). ", | ||
| category=DeprecationWarning | ||
| ) |
There was a problem hiding this comment.
| except TypeError: | |
| warnings.warn( | |
| message="derivative_cb_pre should accept (controls, parameters)." | |
| "Falling back to deprecated signature (controls). ", | |
| category=DeprecationWarning | |
| ) | |
| except TypeError as err: | |
| raise TypeError( | |
| "derivative_cb_pre should accept (controls, parameters)." | |
| ) from err |
There was a problem hiding this comment.
change the default callbacks to take (controls, parameters=None)
| derivatives, | ||
| values) | ||
| values, | ||
| getattr(self, "_parameters", None), |
There was a problem hiding this comment.
Change callback signature depending on parameters like the others.
(and raise informative type error).
| eval_cb_pre=eval_cb_pre, | ||
| eval_cb_post=eval_cb_post, | ||
| derivative_cb_pre=derivative_cb_pre, | ||
| derivative_cb_post=derivative_cb_post, | ||
| hessian_cb_pre=hessian_cb_pre, | ||
| hessian_cb_post=hessian_cb_post, | ||
| tlm_cb_pre=tlm_cb_pre, | ||
| tlm_cb_post=tlm_cb_post, |
There was a problem hiding this comment.
Callbacks for the inner RF should all be None because the inner one doesn't know about controls vs parameters, but the callback signature does.
There was a problem hiding this comment.
Generally good, just the points discussed:
- Don't do unnecessary work to throw away the result.
- Minimal scope for dropping into the inner RF (only for the computation). Keep the controls vs parameters information to do the callbacks etc.
- 0 parameters shouldn't be so special. Just have an empty list rather than a non-existent attribute.
- Remove any changes that are purely formatting so it's clearer what the actual changes are.
|
|
||
|
|
||
| def _call_derivative_cb_pre(cb, controls, parameters=None): | ||
| """Call `derivative_cb_pre` with (controls, parameters) if parameters are passd, otherwise preserve backwards |
There was a problem hiding this comment.
| """Call `derivative_cb_pre` with (controls, parameters) if parameters are passd, otherwise preserve backwards | |
| """Call `derivative_cb_pre` with (controls, parameters) if parameters are passed, otherwise preserve backwards |
…d parameter in check in `__call__` in favour of a more informative error message
3 participants
Current method to use parameters
The
derivative_componentsoptional argument ofReducedFunctionalis used after adding parameters to the list of controls, to specify which components are to be zeroed out (by omitting them fromderivative_components). This allows the user to update parameters by calling the Reduced Functional while zeroing out the gradient with respect to the parameters.Parametrised Reduced Functional
ParametrisedReducedFunctionalis a subclass ofReducedFunctionalwith wrappingcallandderivativemethods, with the parameters as attributes. Theparameter_updatemethod is called to update parameters. The parameters are not included in the derivative calculation and the optional argumentderivative_componentsis not required.