Constellation checkpointing #129
Conversation
| * and linearly over time. | ||
| * @return std::vector<Particle> : satellites to be added to the simulation. | ||
| */ | ||
| std::vector<Particle> tick(); | ||
| std::vector<Particle> tick(size_t simulationTime); |
There was a problem hiding this comment.
doc for new parameter missing
| // if checkpoint is loaded, throw away already inserted satellites | ||
| if (config.defines("io/hdf5/checkpoint/file")) { | ||
| tick(config.get<size_t>("io/hdf5/checkpoint/iteration", -1, true)); | ||
| } |
There was a problem hiding this comment.
Could you add some more explanatory comments here? I don't fully get this. So if a checkpoint is used you call tick() but if no iteration of the checkpoint is defined you pass -1 which is actually 2^64 -1 because it is a size_t. In tick() we will hit the inactive case because we just initialized status. There you then cast this back to a signed long, hence the if in Line 111 may or may not be true.
There was a problem hiding this comment.
I needed the startingIteration as it is in the simulation class, but instead of passing it, I copied what was assigned to startingIteration. I see now though that this is false, because the fallback value of -1 used in Simulation would be added +1 and would then be 0 (right?). I mean this snippet:
const auto startingIteration = config.defines("io/hdf5", true) ? config.get<size_t>("io/hdf5/checkpoint/iteration", -1, true) + 1 : 0;
Is there actually a case where the -1 (+1) becomes relevant? Because if a checkpoint is used, but no iteration is given, the highest iteration found in the checkpoint is always loaded right?
There was a problem hiding this comment.
I think I casted simulationTime to long, because startTime can be negative (if insertion started before referenceTime or before the startingIteration when a checkpoint is loaded) and the negative number was then evaluated to be bigger than simulationTime since both numbers were apparently treated as size_t numbers. So the if in Line 111, when loading a checkpoint, is true if the loaded iteration is later than the scheduled start of the constellation (insertion started during simulation that produced the loaded checkpoint)
| @@ -77,11 +78,13 @@ class Simulation { | |||
| * parameter! | |||
| * @param constellationCutoff range parameter for checked insertion: if the insertion would be within a distance | |||
| * of constellationCutoff to any other object the insertion is delayed instead | |||
| * @param simulationTime the current iteration | |||
There was a problem hiding this comment.
why don't you rename the parameter to iteration then?
There was a problem hiding this comment.
I wanted to more stress that this is in the time unit of the simulation. But I think you are right iteration also makes it clear and is less confusing :D
|
|
||
| // add ids of previous checkpoint to list of existing particle ids in order to avoid adding duplicate | ||
| // constantProperties | ||
| for (auto &id : alreadyExistingIds) { |
There was a problem hiding this comment.
| for (auto &id : alreadyExistingIds) { | |
| for (const auto &id : alreadyExistingIds) { |
| if (addedConstantPropertiesIds.find(id) == addedConstantPropertiesIds.end()) { | ||
| addedConstantPropertiesIds.insert(id); |
There was a problem hiding this comment.
isn't this potentially expensive? We do this if for every particle (O(N)) and the find is in O(log(M)) where M==addedConstantPropertiesIds.size() which is >=N.
For now I would be ok with merging this but please add a comment here that this might need improvement.
There was a problem hiding this comment.
Yes, it is :( I can add a comment
| @@ -49,10 +52,10 @@ class HDF5Writer final : public ConjuctionWriterInterface { | |||
|
|
|||
| private: | |||
| /** | |||
| * Highest partilce ID that was written in any previous iteration. | |||
| * For anything below this ID constant particle properties are already written. | |||
| * Contains particle IDs in order to add a particles constantProperties only once. | |||
There was a problem hiding this comment.
| * Contains particle IDs in order to add a particles constantProperties only once. | |
| * Set of IDs that are already written to the file. |
Is this correct? Maybe rename it to writtenIDs ?
There was a problem hiding this comment.
This is technically not true during the execution of writeParticles(), but it doesn't really matter right? Should I apply this suggestion?
Description
Test
Test simulation with 3 constellations with checkpointing (one added before checkpoint, second is interrupted by simulation end, third is added after checkpoint)