_set_timestep and get_timesteps for in CheckpointFile

[sghelichkhani]

Background

Previously, with DumbCheckpoint, we had set_timestep and get_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 have set_timestep and get_timesteps, primarily because of the way we are handling indexing using PETSc's ViewerHDF5 SetTimestep and IncrementTimestep. This PR introduces _set_timestep and get_timesteps to CheckpointFile. The implementation tries to be consistent with PETSc's SetTimestep and IncrementTimestep. Specifically, _set_timestep in CheckpointFile is invoked only when a timetag is given to save_function.

[dham]

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.

[connorjward]

Thanks for the contribution!

[ksagiyam]

As David suggested, could you please add tests?

[sghelichkhani]

As David suggested, could you please add tests?

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.

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.

[connorjward]

I think that this looks OK but @ksagiyam should probably be the person to pass judgement.

@ksagiyam (not for this PR) would it be possible to generalise this to work for arbitrary attributes? Something like save_function(f, **kwargs) where kwargs is somehow unpacked and serialised.

[ksagiyam]

I started feeling that designing timestepping IO is worth a bit more thoughts.

Could you first look at the suggested changes?

[ksagiyam]

@connorjward Yes, I think so. Technically, users have access to h5pyfile and methods to construct paths, so they can save whatever they want, but it might be worth making a higher level interface available.

[connorjward]

@connorjward Yes, I think so. Technically, users have access to h5pyfile and methods to construct paths, so they can save whatever they want, but it might be worth making a higher level interface available.

@ksagiyam should I create an issue (labelled "enhancement") for this or do you want to tackle this in this PR? You said:

I started feeling that designing timestepping IO is worth a bit more thoughts.

[ksagiyam]

@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 timestepping_indices should be regarded as a topological property while timestepping_ts (or whatever we call it) should be a "geometric" one (these are coordinates in the temporal direction). I think I need to summarise my thoughts first.

[sghelichkhani]

I think we should actually only allow users to save with t=None all the time or t=some_real_value all the time. This will let us avoid defining DEFAULT_REAL and avoid storing redundant values in the file. (Same for timestep). 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 of PREFIX_TIMESTEPPING + "times"/PREFIX_TIMESTEPPING + "timesteps" as putting s for plural is not always correct? We then enforce the keys of timestepping_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.

[ksagiyam]

I think it looks great. Suggested are some final touch ups.

[ksagiyam]

@sghelichkhani We should probably push all timestepping attributes to self._path_to_vec_timestepping/self._path_to_function_timestepping folders just as we do for extrusion, but this should be simple. Let me do that after you review my current suggestions.

[sghelichkhani]

Thanks @ksagiyam. Looks good to me. Once you do the final change I am happy for this to go.

[ksagiyam]

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.

[sghelichkhani]

Done! Let me know if I should do a squash on my end.

[ksagiyam]

Thanks. I will start CI.