Skip to content

Conversation

@bangerth
Copy link
Contributor

This is part of #6744: The Postprocessors::Topography class stores state, but does not actually serialize it during checkpointing. This is a bug.

Since this is the first of the patches that actually fix something for #6744: I have a hard time thinking about how we would create good tests for these kinds of things. We have some tests for checkpointing, but for these postprocessors we'd have to run them for long enough that they create output, checkpoint, restart, run long enough that they create more output, and then check that there is an observable change in output. That require looking into each of these postprocessor classes to see what they actually do, find a test that exercise it, adjust it to the (complicated) structure we use to test checkpointing, and make something of that. That will be a lot of work. I'm not sure that's worth it -- thoughts?

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks ok.

Concerning the tests: I understand what you mean, the checkpoint tests are very tricky to get right and most likely unnecessary for simple cases. Would it be possible to write a unit test like this:

Topography<dim> test_postprocessor;
std::map<std::string, std::string> archive;
test_postprocessor.save (archive);

This would just serialize a default state, but maybe that is better than nothing? I am just not sure what then to do with the archive string, since it is likely uncomprehensible binary data. Would this be system dependent? If not, maybe we can store a reference string in the unit test? This would be a nice system that also applies to other serialization tests. However, if it doesnt work for one reason or another, we could also just leave out the test for this type of operation.

@bangerth
Copy link
Contributor Author

I'll think about that some more once I'm back home from having to deal with family issues next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants