-
Notifications
You must be signed in to change notification settings - Fork 159
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
_set_timestep
and get_timesteps
for in CheckpointFile
#3310
_set_timestep
and get_timesteps
for in CheckpointFile
#3310
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this addition.
I'll let @ksagiyam look at this in detail, but I would point out that it does need a test. I would suggest writing some trivial time steps to a file. Re-opening that file and then using this API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As David suggested, could you please add tests?
Co-authored-by: Connor Ward <c.ward20@imperial.ac.uk> Co-authored-by: ksagiyam <46749170+ksagiyam@users.noreply.github.com>
…t for reading in and out
Added a simple checkpointing case of a mixed function, and then loading one of its subfunctions and making sure all work as expected. As @ksagiyam suggested, making it work for mixed functions required a bit of change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started feeling that designing timestepping IO is worth a bit more thoughts.
Could you first look at the suggested changes?
@connorjward Yes, I think so. Technically, users have access to |
@ksagiyam should I create an issue (labelled "enhancement") for this or do you want to tackle this in this PR? You said:
|
@connorjward I think that can be done in a separate PR. Please go ahead and open an issue about it. I would like to think a bit further about the design/file-structure. For instance, I think |
Co-authored-by: ksagiyam <46749170+ksagiyam@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should actually only allow users to save with
t=None
all the time ort=some_real_value
all the time. This will let us avoid definingDEFAULT_REAL
and avoid storing redundant values in the file. (Same fortimestep
).indices
should always be stored, I think.If we are to extend the interface as suggested by @connorjward in the future (#3328), what we do here must be consistent. Will the interface look like:
save_function(f, idx=..., timestepping_info={"time": ..., "timestep": ...})
? If so, I think, the corresponding attribute names should be
PREFIX_TIMESTEPPING + "_history" + "_time"
/PREFIX_TIMESTEPPING + "_history" + "_timestep"
(or something) instead ofPREFIX_TIMESTEPPING + "times"
/PREFIX_TIMESTEPPING + "timesteps"
as puttings
for plural is not always correct? We then enforce the keys oftimestepping_info
dictionary to be the same throughout timestepping as in the above.
Thanks @ksagiyam. So to be consistent with the idea in #3328 I changed the timestepping_info to a dictionary as suggested. Then depending on whatever arbitrary key timestepping info is stored, the user can retrieve the same information within a dictionary. For the default case in timestepping mode, only {'indices':[list of indices]}
are returned by get_timestepping_history
. The same can be actually used for whatever float
the user needs to store along a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks great. Suggested are some final touch ups.
@sghelichkhani We should probably push all timestepping attributes to |
Co-authored-by: ksagiyam <46749170+ksagiyam@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ksagiyam. Looks good to me. Once you do the final change I am happy for this to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put:
def _path_to_vec_timestepping(self, tmesh_name, dm_name, tf_name):
return os.path.join(self._path_to_vec(tmesh_name, dm_name, tf_name), PREFIX_TIMESTEPPING)
after def _path_to_vec(...)
and:
def _path_to_function_timestepping(self, tmesh_name, mesh_name, V_name, function_name):
return os.path.join(self._path_to_function(tmesh_name, mesh_name, V_name, function_name), PREFIX_TIMESTEPPING)
after def _path_to_function(...)
.
Please replace l.958:
path = self._path_to_vec(tmesh.name, dm.name, tf.name())
with:
dm_name = dm.name
path = self._path_to_vec(tmesh.name, dm_name, tf.name())
Please also make your branch up-to-date.
Co-authored-by: ksagiyam <46749170+ksagiyam@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Let me know if I should do a squash on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will start CI.
Background
Previously, with
DumbCheckpoint
, we hadset_timestep
andget_timesteps
functions which we were using for checkpointing states over time, allowing the user to retrieve all the stored indices and timesteps from a checkpoint.Issue and proposed change
The transition to
CheckpointFile
, we don't haveset_timestep
andget_timesteps
, primarily because of the way we are handling indexing using PETSc's ViewerHDF5SetTimestep
andIncrementTimestep
. This PR introduces_set_timestep
andget_timesteps
toCheckpointFile
. The implementation tries to be consistent with PETSc'sSetTimestep
andIncrementTimestep
. Specifically,_set_timestep
inCheckpointFile
is invoked only when a timetag is given tosave_function
.