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.

Sign up for GitHub

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

Make t and n optional arguments in store_checkpoint #178

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Make t and n optional arguments in store_checkpoint #178

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
041d207
Make t and n optional arguments in store_checkpoint
NiklasVin Aug 29, 2024
4a865cf
Add integration tests
NiklasVin Nov 6, 2024
3c776c6
Add changelog entry.
BenjaminRodenberg Sep 5, 2024
803e64f
Remove checks for FEniCS and python3 (#182)
BenjaminRodenberg Sep 5, 2024
bdefd9a
Modify `setup.py` to avoid crash due to version 4.0.0 of `mpi4py` dur…
NiklasVin Sep 7, 2024
efc995d
Clarify installation procedure (#183)
NiklasVin Oct 31, 2024
c7d5924
Clarify how to merge in ReleaseGuide.md
BenjaminRodenberg Oct 31, 2024
f3d8189
bump version
BenjaminRodenberg Oct 25, 2024
a90f9ad
Include creation of post-tag bump like in python bindings.
BenjaminRodenberg Oct 31, 2024
debb274
Fix bare links.
BenjaminRodenberg Nov 7, 2024
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
23 changes: 17 additions & 6 deletions fenicsprecice/fenicsprecice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,17 +432,17 @@ def initialize(self, coupling_subdomain, read_function_space=None, write_object=

self._participant.initialize()

def store_checkpoint(self, payload, t, n):
def store_checkpoint(self, payload, t = None, n = None):
"""
Defines an object of class SolverState which stores the current state of the variable and the time stamp.

Parameters
----------
payload : fenics.function or a list of fenics.functions
Current state of the physical variable(s) of interest for this participant.
t : double
t : double (optional)
Current simulation time.
n : int
n : int (optional)
Current time window (iteration) number.
"""
if self._first_advance_done:
Expand All @@ -459,14 +459,25 @@ def retrieve_checkpoint(self):
-------
u : FEniCS Function
Current state of the physical variable of interest for this participant.
t : double
t : double (optional)
Current simulation time.
n : int
n : int (optional)
Current time window (iteration) number.
"""
assert (not self.is_time_window_complete())
logger.debug("Restore solver state")
return self._checkpoint.get_state()

# 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
Comment on lines +470 to +480
Copy link
Member

Choose a reason for hiding this comment

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

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).


def advance(self, dt):
"""
Expand Down
Loading