Implement state checkpointing for decision nodes - #587
Conversation
6df7006 to
953304e
Compare
b4679c2 to
214c5e2
Compare
This avoids using GCC11 which had some bugs in their ranges implementation.
86b97e8 to
9cd035b
Compare
f1aa563 to
06b9dc5
Compare
06b9dc5 to
f70699e
Compare
30637e6 to
5fbe15b
Compare
5fbe15b to
4402568
Compare
| CHECK_THAT(inode_ptr->view(state), RangeEquals({8, 8, 8, 8, 8, 8, -3, 3})); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
A bit of a pain, but I think it might be worth adding a single test for BinaryNode with checkpointing to check that all the correct indices are cached with 1s and 0s (I am referring to the BinaryNode DisjointSparseSet).
THEN("Sum constraint sums, tracked indices, and state are correct") {
CHECK(bnode_ptr->sum_constraints_lhs(state).size() == 1);
CHECK(bnode_ptr->sum_constraints_lhs(state).data()[0].size() == 1);
CHECK_THAT(bnode_ptr->sum_constraints_lhs(state)[0], RangeEquals({3}));
check_indices<true>(state, bnode_ptr, 0, 0, {1, 2, 5}); // <- ***** This check is the important one ******
check_indices<false>(state, bnode_ptr, 0, 0, {0, 3, 4}); // <- ***** And this one ******
CHECK_THAT(bnode_ptr->view(state), RangeEquals(expected_init));
}There was a problem hiding this comment.
It seems that BinaryNode::assign_checkpoint() is very broken because it has a different update method. I think we need to refactor the various state data classes to use virtual methods.
|
I think I need to take another documentation pass. I'll try to do that today |
fastbodin
left a comment
There was a problem hiding this comment.
Officially gone through everything. I need to look at the number.cpp and collections.cpp again. On the whole, looks good. Most of my comments above are clarifications. I will resolved them ASAP to avoid clogging the thread on this PR.
|
|
||
| auto* checkpoint_ptr = static_cast<NumberNodeCheckpoint_*>(checkpoint.get()); | ||
|
|
||
| // todo: assert that this checkpoint is the latest |
There was a problem hiding this comment.
Testing my understanding. You mean
// Right now, you can only revert to the most recent checkpoint. It's
// pretty straightforward to support going further back, but this is all
// we need right now.
assert(this->checkpoint_ptr<NumberNodeCheckpoint_>() == checkpoint_ptr);| // The current "drop". The drop is used when a checkpoint is created while | ||
| // a node has some mutations already applied. This tells the checkpoint | ||
| // how to handle the diff associated with those mutations, i.e., the ones | ||
| // the checkpoint shouldn't be tracking. |
There was a problem hiding this comment.
Nit: This doc string is good but omits some information. Possible alternative (that could be improved) based on my (possibly incorrect) understanding of drop.
// The current `drop` where `drop` is equal to the number of `Updates`
// in the last `diff` cached by `updates_` that the checkpoint
// is NOT responsible for tracking. Functionally, there are two cases.
// 1. `drop == 0` and the checkpoint is responsible for all `Updates`
// stored in the last `diff` of `updates_`.
// 2. `drop > 0` and the checkpoint is *not* responsible for the last
// `drop` number of `Updates` in the the last `diff` of `updates_`.There was a problem hiding this comment.
I just realized the above was in-fact, incorrect. Will try again.
| const auto& [idx, old, _] : | ||
| state_data->diff() | std::views::reverse | std::views::take(excess_updates) | ||
| ) { | ||
| state_data->set(idx, old); |
There was a problem hiding this comment.
Looking at this again, this is a bug. This does not take into account the running slice sums (should they be tracked). This was not caught in the tests because the relevant tests use exchange() which does not affect the running slice sums`.
Closes #510
Closes #552 by replacing it
AI Generation Disclosure
No AI was used to write the code. I intend to use it to review this PR.