Serialize the dynamic core boundary temperature plugin#6918
Serialize the dynamic core boundary temperature plugin#6918Francyrad wants to merge 3 commits intogeodynamics:mainfrom
Conversation
bangerth
left a comment
There was a problem hiding this comment.
The addition of the serialize(), save(), and load() functions is undoubtedly correct. I must admit that I don't understand why the rest is necessary. Can you explain?
In principle, loading from a checkpoint should just re-create the exact same state we had before. There shouldn't be a need to store something like a flag that describes whether we are just resuming.
| core_data = {}; | ||
| is_first_call = true; | ||
| core_data.is_initialized = false; |
There was a problem hiding this comment.
Is this part unrelated to the current purpose of getting things to work to work with serialization? I don't see the connection, but I also don't see how it's supposed to work: core_data = {} does not actually initialize anything (I think) because the CoreData class has no constructor :-(
There was a problem hiding this comment.
Is this part unrelated to the current purpose of getting things to work to work with serialization? I don't see the connection, but I also don't see how it's supposed to work:
core_data = {}does not actually initialize anything (I think) because theCoreDataclass has no constructor :-(
This was related to making the restart path deterministic.
Before this patch, only core_data.is_initialized was explicitly set in the constructor, while the other members of core_data were left uninitialized until later. Since the restart path now relies on core_data before the normal prm-based initialization path, I wanted the whole struct to start from a well-defined zero state.
My understanding is that core_data = {} does value-initialize the aggregate and therefore zero-initialize all scalar members, even though CoreData has no user-defined constructor. But I agree that this is not very explicit and can easily be misread.
There was a problem hiding this comment.
It's never clear to me what happens with default initialization of members of type double or int or bool. Let me just write a constructor for the CoreData structure, so that we don't have to guess here. I think it's right to just split this part of the patch off to a separate one.
Thanks, that is a fair point. The intent was not to store a persistent “we are resuming now” flag. What I actually wanted to serialize is only the persistent state of the core solver itself: The extra logic in |
|
I see. I think there are two options:
Do either of these options seem reasonable? |
|
I think that the issue with initialization is more complicated, see #6924. If you want, take these issues out of the pull request and only add the three serialization functions, then we can merge this one independently. |
|
I can start to work on it Tuesday |
This PR serializes the state of the dynamic core boundary temperature plugin so checkpoint/restart preserves its internal state across runs.
It also keeps compatibility with older checkpoints by falling back to the legacy restart data stored in the core statistics postprocessor.
Refs #6744.