Enable adjoint #607
Enable adjoint #607
Conversation
There was a problem hiding this comment.
Sorry it's taken me so long to look at this! It's really exciting to have this so thanks very much for getting it working.
Two main questions:
- maybe you already discounted this for a good reason, but we already set floats to be
Constants by default in ourConfigurationobject. Should we just set them straightaway to being real functions there? - One for @jshipton really -- how should the notebook fit with the other notebooks PR Jupyter notebooks #294? It might be that we can cut down on a lot of the explanation of the Gusto code and focus the notebook in this PR on the adjoint? We could potentially move the notebook into this PR into the notebooks PR
| @@ -167,7 +167,7 @@ class ShallowWaterParameters(Configuration): | |||
| class WrapperOptions(Configuration, metaclass=ABCMeta): | |||
| """Base class for specifying options for a transport scheme.""" | |||
|
|
|||
| @abstractproperty | |||
| @abstractmethod | |||
There was a problem hiding this comment.
Should this actually just be @property?
| @@ -308,3 +308,17 @@ def check_options(self): | |||
| raise ValueError( | |||
| "Cannot provide both fixed_subcycles and subcycle_by_courant" | |||
| + "parameters.") | |||
|
|
|||
|
|
|||
| def convert_parameters_to_real_space(parameters, mesh): | |||
There was a problem hiding this comment.
Naive question, but can we do this conversion from Constant to real function when instantiating the Configuration object? Or is there a reason we need to do it later on?
There was a problem hiding this comment.
i.e. could we do that conversion here:
gusto/gusto/core/configuration.py
Line 87 in d2fe122
Constants? Can we just do straight to real functions?
There was a problem hiding this comment.
Isn't the problem that we need the mesh? Maybe we just pass that to the Configuration class?
There was a problem hiding this comment.
Ah yes, the problem is that we need the mesh. I think it probably would be better to pass mesh to the Configuration class then
There was a problem hiding this comment.
Obviously that unfortunately would make this a bigger change as all examples would need updating!
There was a problem hiding this comment.
Yes, I think that might be why we didn't want to do it... it does feel like a better solution though
| # in the parameters to a function in real space. This conversion is a | ||
| # preventive to avoid issues with adjoint computations, particularly | ||
| # when the parameters are used as controls in sensitivity analyses. | ||
| convert_parameters_to_real_space(parameters, domain.mesh) |
There was a problem hiding this comment.
So if we did the conversion to reals initially, then we wouldn't need to do this in any of the equations?
| @@ -109,6 +114,7 @@ def __init__(self, domain, parameters, sponge_options=None, | |||
| u_trial = split(self.trials)[0] | |||
| _, rho_bar, theta_bar = split(self.X_ref)[0:3] | |||
| zero_expr = Constant(0.0)*theta | |||
| # Check this for adjoints | |||
There was a problem hiding this comment.
Is this is a leftover comment?
Yes, I think that moving the notebook is a good idea - then I can consider them together as a whole. |
This branch is properly rebased to the main branch.