Make t and n optional arguments in store_checkpoint #178
Conversation
There was a problem hiding this comment.
Since I expect that this is a quite niche feature: A test would be important.
Additionally: In #59 (comment) I mention some tutorials where we could just skip t and/or n. Let's prepare a PR where we actually implement this in the tutorials. This simplifies the tutorials and also acts as an integration test for this feature.
Co-authored-by: Frédéric Simonis <[email protected]>
…ing installation (precice#181) * Require mpi4py < 4 --------- Co-authored-by: Benjamin Rodenberg <[email protected]>
* Add note regarding the externally-managed-environment error * Provide commands that we use in tutorials. --------- Co-authored-by: Benjamin Rodenberg <[email protected]>
| # since t and n are optional, they should not be returned, if not specified | ||
| payload, t, n = self._checkpoint.get_state() | ||
| match (t, n): | ||
| case (None, None): | ||
| return payload | ||
| case (_, None): | ||
| return payload, t | ||
| case (None, _): | ||
| return payload, n | ||
| case _: | ||
| return payload, t, n |
There was a problem hiding this comment.
I just tested this PR with the perpendicular flap. Generally, it seems to work. But I'm not so happy with the implications on the return value of the retrieve_checkpoint function: If I call
cp, t_or_n = adapter.retrieve_checkpoint()
the second argument could be anything. The actual return value depends on how I called store_checkpoint before. This does not look right to me. Sorry to bring this up so late.
Non-breaking solution
As a non-breaking solution, I can only imagine returning None, if t or n has not been provided previously. This results in always calling
cp, t, n = adapter.retrieve_checkpoint()
this would be independent from what we did with store_checkpoint before.
Breaking solutions
I think a more reasonable way would be the following:
cp = adapter.retrieve_checkpoint()
and then
value = cp.value
t = cp.t
n = cp.n
Meaning: cp would be a named tuple (see https://stackoverflow.com/questions/2970608/what-are-named-tuples-in-python). Scipy uses something similar in integrate.solve_ivp and calls it a "bunch object" (see https://docs.scipy.org/doc/scipy/reference/generated/scipy.integrate.solve_ivp.html).
This brings us to the next question: Why not just use the following API:
store_checkpoint(self, payload: namedtuple)
retreive_checkpoint() -> namedtuple
We would then have to allow for FEniCS Functions or simple float or int values in the payload. But this would be an implementation detail and the user just gets back whatever was provided in the original payload.
Important about both proposed solutions: They would be breaking (and therefore, we can only release them with fenicsprecice 3.0.0).
What to do
I would suggest to implement and merge the non-breaking solution. We can open an issue describing the breaking solution and schedule it for version 3.0.0 (probably this will happen not too soon).
Closes #73.