-
Notifications
You must be signed in to change notification settings - Fork 139
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.
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
Simplify checkpointing, change defaults, fix legacy, implement in batched #4646
Conversation
Based on the documentation the default value of checkpoint is -1, should it be better to have 0 as default, namely end of driver section? |
I agree. Pushed change and will update PR text |
Test this please |
… into removestoreconfigs
@prckent your fix brings me attention that we don't have the corresponding tests for batched drivers. |
Agree we are missing some tests here. Which were you thinking of? batched dmc restart tests? or something else? I don't think that we have any tests on checkpoints saved within a section (as opposed to at the end), whether legacy or batched. The restart tests use the "checkpoints" saved at the end of a section. It is not obvious how to reliably catch the checkpoints written within a section for testing. |
That types of tests can only be made on the unit test side. We can simply run a driver with coordinates dumping in the middle, then load it back without using a driver and check against fixed numbers. |
Test this please |
Proposed changes
This PR aims to simplify and improve the checkpointing process.
As mentioned in #4633 the documented every N blocks checkpointing feature was not working as expected. In legacy, this required storeconfigs to also be specified to work (undocumented/unexpected). Checkpointing was also not implemented in the batched drivers.
Here:
Have verified by eye that the checkpoints look sensible and are updated periodically. Open to testing suggestions.
Batched code does not create a info.xml as part of checkpoint.
Also remove unused FastGrad option.
What type(s) of changes does this code introduce?
Does this introduce a breaking change?
What systems has this change been tested on?
sulfur gcc build only. PR for CI.
Checklist
Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.