From 953304e70fea92d41770c2d14d0a03870bfb3091 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Fri, 17 Jul 2026 16:20:56 -0700 Subject: [PATCH 01/21] Implement state checkpointing for CollectionNode --- .../include/dwave-optimization/graph.hpp | 5 + .../dwave-optimization/nodes/collections.hpp | 7 + .../include/dwave-optimization/state.hpp | 16 ++ dwave/optimization/src/graph.cpp | 5 + dwave/optimization/src/nodes/collections.cpp | 154 +++++++++++++++- ...eature-checkpointing-b770d2f2b66f648d.yaml | 7 + tests/cpp/nodes/test_collections.cpp | 169 ++++++++++++++++++ 7 files changed, 362 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/feature-checkpointing-b770d2f2b66f648d.yaml diff --git a/dwave/optimization/include/dwave-optimization/graph.hpp b/dwave/optimization/include/dwave-optimization/graph.hpp index 22fbc3d6..b05baa0e 100644 --- a/dwave/optimization/include/dwave-optimization/graph.hpp +++ b/dwave/optimization/include/dwave-optimization/graph.hpp @@ -151,6 +151,11 @@ class Graph { std::function accept = [](const Graph&, State&) { return true; } ) const; + /// Propagate any pending changes to all nodes in the graph and commit them. + void propose(State& state) const; + // dev note: the name is a bit funny in this case, but we essentially want + // an overload for a "default" `sources` and `accept`. + /// Initialize the state of the given node and all predecessors recursively. static void recursive_initialize(State& state, const Node* ptr); /// Reset the state of the given node and all successors recursively. diff --git a/dwave/optimization/include/dwave-optimization/nodes/collections.hpp b/dwave/optimization/include/dwave-optimization/nodes/collections.hpp index 48b20119..68aeafae 100644 --- a/dwave/optimization/include/dwave-optimization/nodes/collections.hpp +++ b/dwave/optimization/include/dwave-optimization/nodes/collections.hpp @@ -32,8 +32,15 @@ class CollectionNode : public ArrayOutputMixin, public DecisionNode { // Set the node's state, tracking the diff. void assign(State& state, std::vector values) const; + /// Set the current state to match the one at the time the given checkpoint was created. + void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const; + void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const; + const double* buff(const State& state) const override; + /// Get a checkpoint, an IOU that can be used to return the node to its current state. + checkpoint_type checkpoint(State& state) const; + void commit(State&) const override; std::span diff(const State& state) const override; diff --git a/dwave/optimization/include/dwave-optimization/state.hpp b/dwave/optimization/include/dwave-optimization/state.hpp index 884bf972..e82b9cbc 100644 --- a/dwave/optimization/include/dwave-optimization/state.hpp +++ b/dwave/optimization/include/dwave-optimization/state.hpp @@ -36,4 +36,20 @@ struct NodeStateData { using State = typename std::vector>; +/// A generic base class for node checkpoints. +struct NodeStateCheckpoint { + NodeStateCheckpoint() = default; + NodeStateCheckpoint(const NodeStateCheckpoint&) = default; + NodeStateCheckpoint(NodeStateCheckpoint&&) = delete; + NodeStateCheckpoint& operator=(const NodeStateCheckpoint&) = default; + NodeStateCheckpoint& operator=(NodeStateCheckpoint&&) = delete; + + virtual ~NodeStateCheckpoint() = default; + + /// Whether the checkpoint is still available to be used. + virtual bool valid() const = 0; +}; + +using checkpoint_type = std::unique_ptr; + } // namespace dwave::optimization diff --git a/dwave/optimization/src/graph.cpp b/dwave/optimization/src/graph.cpp index 733e94ae..a625cd40 100644 --- a/dwave/optimization/src/graph.cpp +++ b/dwave/optimization/src/graph.cpp @@ -195,6 +195,11 @@ void Graph::propose( } } +void Graph::propose(State& state) const { + propagate(state); + commit(state); +} + void Graph::recursive_initialize(State& state, const Node* ptr) { ssize_t index = ptr->topological_index(); diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index 0ea4f805..5dbb4d8b 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -14,9 +14,11 @@ #include "dwave-optimization/nodes/collections.hpp" +#include #include #include #include +#include namespace dwave::optimization { @@ -66,6 +68,51 @@ std::vector augment_collection_(std::vector values, const ssize_ return values; } +class CollectionStateData_; + +class CollectionCheckpoint_ : public NodeStateCheckpoint { + public: + CollectionCheckpoint_() = delete; + + CollectionCheckpoint_(CollectionStateData_* state_ptr); + + ~CollectionCheckpoint_() override; + + // detach the updates as a flattened view (in the forward order) + auto detach_updates() { + auto updates = std::move(updates_) | std::views::join; + assert(updates_.empty()); + return updates; + } + + ssize_t& drop() { return drop_; } + ssize_t drop() const { return drop_; } + + void emplace_updates(std::vector updates) { + if (drop_) { + // In C++23 we could use assign_range() which would be nicer + auto relevant = updates | std::views::drop(drop_); + updates_.emplace_back(relevant.begin(), relevant.end()); + drop_ = 0; + } else { + updates_.emplace_back(std::move(updates)); + } + } + + ssize_t size() { return size_; } + + bool valid() const override { return true; } + + private: + std::vector> updates_; + ssize_t drop_; + + ssize_t size_; + + CollectionCheckpoint_* older_checkpoint_ptr_; + std::variant newer_checkpoint_ptr_; +}; + class CollectionStateData_ : public NodeStateData { public: explicit CollectionStateData_(ssize_t n) : CollectionStateData_(n, n) {} @@ -109,11 +156,54 @@ class CollectionStateData_ : public NodeStateData { assert(this->size_ == size); } + void assign(std::unique_ptr& checkpoint) { + // convert the checkpoint into something we can read + auto* checkpoint_ptr = static_cast(checkpoint.get()); + + // 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(older_checkpoint_ptr_ == checkpoint_ptr); + + // Ok, let's get ourselves to the same place as the checkpoint + + // we want to minimize the size of the visible buffer, so let's shrink ourselves + // if we need to + while (size_ > checkpoint_ptr->size()) shrink(); + + for (const auto& [idx, old, _] : checkpoint_ptr->detach_updates() | std::views::reverse) { + if (elements_[idx] == old) continue; // nothing to do + + all_updates_.emplace_back(idx, elements_[idx], old); + if (idx < size_) updates_.emplace_back(idx, elements_[idx], old); + + elements_[idx] = old; + } + + // now that we've filled in our buffer, grow until we're the correct size + while (size_ < checkpoint_ptr->size()) grow(); + + // update the "drop" value of the checkpoint so that our next commit doesn't + // add all of the changes we just added + checkpoint_ptr->drop() = all_updates_.size(); + } + const double* buff() const { return elements_.data(); } + std::unique_ptr checkpoint() { + return std::make_unique(this); + } + void commit() { updates_.clear(); - all_updates_.clear(); + + if (older_checkpoint_ptr_ != nullptr) { + older_checkpoint_ptr_->emplace_updates(std::move(all_updates_)); + assert(all_updates_.empty()); + } else { + all_updates_.clear(); + } + previous_size_ = size_; } @@ -213,8 +303,52 @@ class CollectionStateData_ : public NodeStateData { // commit/revert ssize_t size_; ssize_t previous_size_; + + friend CollectionCheckpoint_; + CollectionCheckpoint_* older_checkpoint_ptr_ = nullptr; }; +CollectionCheckpoint_::CollectionCheckpoint_(CollectionStateData_* state_ptr) : + updates_(), + drop_(state_ptr->all_updates_.size()), // so we ignore any updates added before we're made + size_(state_ptr->size()), + older_checkpoint_ptr_(state_ptr->older_checkpoint_ptr_), + newer_checkpoint_ptr_(state_ptr) { + if (older_checkpoint_ptr_ != nullptr) { + older_checkpoint_ptr_->newer_checkpoint_ptr_ = this; + + if (not state_ptr->all_updates_.empty()) { + older_checkpoint_ptr_->emplace_updates(state_ptr->all_updates_); // copy! + } + assert(older_checkpoint_ptr_->drop() == 0); + } + + std::get(newer_checkpoint_ptr_)->older_checkpoint_ptr_ = this; +} + +CollectionCheckpoint_::~CollectionCheckpoint_() { + if (older_checkpoint_ptr_ == nullptr) { + // We're the oldest checkpoint, so delete ourselves from the newer one + // and let any information we're holding die with us + std::visit([](auto* ptr) { ptr->older_checkpoint_ptr_ = nullptr; }, newer_checkpoint_ptr_); + } else { + // We're an intermediate checkpoint, so we need to pass any information we're + // holding to the next oldest checkpoint and update the pointers on either side of + // us + assert(older_checkpoint_ptr_->drop_ == 0); + for (std::vector& updates : updates_) { + older_checkpoint_ptr_->emplace_updates(std::move(updates)); + } + older_checkpoint_ptr_->drop_ = drop_; + + older_checkpoint_ptr_->newer_checkpoint_ptr_ = newer_checkpoint_ptr_; + std::visit( + [&](auto* ptr) { ptr->older_checkpoint_ptr_ = older_checkpoint_ptr_; }, + newer_checkpoint_ptr_ + ); + } +} + CollectionNode::CollectionNode(ssize_t max_value, ssize_t min_size, ssize_t max_size) : ArrayOutputMixin((min_size == max_size) ? max_size : Array::DYNAMIC_SIZE), max_value_(max_value), @@ -242,6 +376,24 @@ void CollectionNode::assign(State& state, std::vector values) const { data_ptr_(state)->assign(std::move(augemented), size); } +void CollectionNode::assign_from_checkpoint( + State& state, + std::unique_ptr& checkpoint +) const { + data_ptr_(state)->assign(checkpoint); +} +void CollectionNode::assign_from_checkpoint( + State& state, + std::unique_ptr&& checkpoint +) const { + assign_from_checkpoint(state, checkpoint); // call the lvalue version + checkpoint.reset(); +} + +std::unique_ptr CollectionNode::checkpoint(State& state) const { + return data_ptr_(state)->checkpoint(); +} + void CollectionNode::commit(State& state) const { data_ptr_(state)->commit(); } diff --git a/releasenotes/notes/feature-checkpointing-b770d2f2b66f648d.yaml b/releasenotes/notes/feature-checkpointing-b770d2f2b66f648d.yaml new file mode 100644 index 00000000..c82063e4 --- /dev/null +++ b/releasenotes/notes/feature-checkpointing-b770d2f2b66f648d.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Add a C++ ``Graph::propose(State&)`` overload that propagates and commits. + - | + Add checkpointing to ``CollectionNode``. + See `#510 `_. diff --git a/tests/cpp/nodes/test_collections.cpp b/tests/cpp/nodes/test_collections.cpp index 4090de04..218d0cff 100644 --- a/tests/cpp/nodes/test_collections.cpp +++ b/tests/cpp/nodes/test_collections.cpp @@ -703,6 +703,175 @@ TEST_CASE("SetNode") { } } } + + GIVEN("A set(5) initialized to {0, 1}") { + auto graph = Graph(); + + auto* set_ptr = graph.emplace_node(5); + + graph.emplace_node(set_ptr); + + auto state = graph.empty_state(); + set_ptr->initialize_state(state, {0, 1}); + graph.initialize_state(state); + + WHEN("We create a checkpoint from the initialized state") { + auto checkpoint0 = set_ptr->checkpoint(state); + + AND_WHEN("The set is changed to {3, 4, 1}") { + set_ptr->assign(state, {3, 4, 1}); + + graph.propose(state); + CHECK(set_ptr->size(state) == 3); + CHECK_THAT(set_ptr->view(state), RangeEquals({3, 4, 1})); + + AND_WHEN("We revert to the checkpoint") { + set_ptr->assign_from_checkpoint(state, checkpoint0); + graph.propose(state); + + THEN("The state has returned to {0, 1}") { + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + } + + AND_WHEN( + "The set is again mutated and then reverted using the same checkpoint" + ) { + set_ptr->assign(state, {4, 1, 0}); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1, 0})); + + set_ptr->assign_from_checkpoint(state, checkpoint0); + graph.propose(state); + + THEN("The state has returned to {0, 1}") { + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + } + } + } + + AND_WHEN("The set is changed to {3, 4, 1} and then the checkpoint is returned") { + set_ptr->assign(state, {3, 4, 1}); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({3, 4, 1})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint0)); + graph.propose(state); + + THEN("The state has returned to {0, 1} and the checkpoint is reset") { + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + CHECK(checkpoint0 == nullptr); + } + } + + AND_WHEN("We create another checkpoint") { + auto checkpoint1 = set_ptr->checkpoint(state); + + AND_WHEN("The set is changed to {4, 1}") { + set_ptr->assign(state, {4, 1}); + graph.propose(state); + + THEN("We can revert to the checkpoints one-by-one") { + set_ptr->assign_from_checkpoint(state, std::move(checkpoint1)); + graph.propose(state); + + CHECK_THAT(set_ptr->view(state), RangeEquals({3, 4, 1})); + + set_ptr->assign_from_checkpoint(state, checkpoint0); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + } + } + } + } + } + + WHEN("We mutate the state and then create a checkpoint before commiting") { + set_ptr->assign(state, {4, 1}); + auto checkpoint = set_ptr->checkpoint(state); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + + set_ptr->assign(state, {3, 4, 1}); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({3, 4, 1})); + + set_ptr->assign_from_checkpoint(state, checkpoint); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + } + + WHEN("We do several mutations and create several checkpoints within the same commit") { + auto checkpoint0 = set_ptr->checkpoint(state); + + set_ptr->assign(state, {4, 1}); + auto checkpoint1 = set_ptr->checkpoint(state); + + set_ptr->assign(state, {3, 4, 1}); + auto checkpoint2 = set_ptr->checkpoint(state); + + set_ptr->assign(state, {4, 2}); + graph.propose(state); // mix a propose in there + auto checkpoint3 = set_ptr->checkpoint(state); + + set_ptr->assign(state, {2}); + auto checkpoint4 = set_ptr->checkpoint(state); + + set_ptr->assign(state, {3, 2, 1, 0}); + graph.propose(state); + + THEN("we can go backwards through them without commiting and everything is correct") { + set_ptr->assign_from_checkpoint(state, std::move(checkpoint4)); + CHECK_THAT(set_ptr->view(state), RangeEquals({2})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint3)); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 2})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint2)); + CHECK_THAT(set_ptr->view(state), RangeEquals({3, 4, 1})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint1)); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint0)); + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + } + + THEN( + "we can go backwards through them and commit each time and everything is correct" + ) { + set_ptr->assign_from_checkpoint(state, std::move(checkpoint4)); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({2})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint3)); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 2})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint2)); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({3, 4, 1})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint1)); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint0)); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + } + + THEN("we can delete some intermediate checkpoints and everything stays valid") { + checkpoint2.reset(); + checkpoint4.reset(); + checkpoint0.reset(); + checkpoint3.reset(); + + set_ptr->assign_from_checkpoint(state, checkpoint1); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + } + } + } } } // namespace dwave::optimization From 214c5e23952649ba5adb24e8c74f3bcffe55e9e2 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Mon, 20 Jul 2026 11:49:23 -0700 Subject: [PATCH 02/21] Fix reverts and dangling pointers for CollectionNode checkpoints --- .../include/dwave-optimization/array.hpp | 3 + dwave/optimization/src/nodes/collections.cpp | 75 +++++++-- tests/cpp/nodes/test_collections.cpp | 143 ++++++++++++++++-- 3 files changed, 196 insertions(+), 25 deletions(-) diff --git a/dwave/optimization/include/dwave-optimization/array.hpp b/dwave/optimization/include/dwave-optimization/array.hpp index 7182b3af..8ece49d1 100644 --- a/dwave/optimization/include/dwave-optimization/array.hpp +++ b/dwave/optimization/include/dwave-optimization/array.hpp @@ -347,6 +347,9 @@ struct Update { // Return true if the update does nothing - that is old and value are the same. bool identity() const { return null() || old == value; } + // Return the update that would undo the current update + Update inverse() const { return Update(index, value, old); } + // Use NaN to represent the "nothing" value used in placements/removals static constexpr double nothing = std::numeric_limits::signaling_NaN(); diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index 5dbb4d8b..0d168ab6 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -88,15 +88,36 @@ class CollectionCheckpoint_ : public NodeStateCheckpoint { ssize_t& drop() { return drop_; } ssize_t drop() const { return drop_; } - void emplace_updates(std::vector updates) { - if (drop_) { - // In C++23 we could use assign_range() which would be nicer - auto relevant = updates | std::views::drop(drop_); - updates_.emplace_back(relevant.begin(), relevant.end()); - drop_ = 0; - } else { + // Track the updates associated with a commit + void commit_updates(std::vector updates) { + assert(0 <= drop_ and static_cast(drop_) <= updates.size()); + + if (not drop_) { updates_.emplace_back(std::move(updates)); + return; } + + // Otherwise we only want to take the updates up to drop + // In C++23 we could use assign_range() which would be nicer + auto relevant = std::move(updates) | std::views::drop(drop_); + updates_.emplace_back(relevant.begin(), relevant.end()); + drop_ = 0; + } + + // Track the updates associated with a revert + void revert_updates(std::vector updates) { + assert(0 <= drop_ and static_cast(drop_) <= updates.size()); + + if (not drop_) return; // nothing to do + + // We want to track the updates that would revert the changes from the + // current state. + // In C++23 we could use assign_range() which would be nicer + auto relevant = std::move(updates) | std::views::take(drop_) | std::views::reverse | + std::views::transform([](const Update& up) { return up.inverse(); }); + updates_.emplace_back(relevant.begin(), relevant.end()); + + drop_ = 0; } ssize_t size() { return size_; } @@ -104,6 +125,8 @@ class CollectionCheckpoint_ : public NodeStateCheckpoint { bool valid() const override { return true; } private: + friend CollectionStateData_; + std::vector> updates_; ssize_t drop_; @@ -130,6 +153,15 @@ class CollectionStateData_ : public NodeStateData { assert(0 <= size_ and static_cast(size_) <= elements_.size()); } + ~CollectionStateData_() { + // make sure if we're destructed before the checkpoint that we clean + // up the dangling pointer + if (older_checkpoint_ptr_ != nullptr) { + older_checkpoint_ptr_->newer_checkpoint_ptr_ = + static_cast(nullptr); + } + } + void assign(std::vector&& values, ssize_t size) { // this should have been checked already by the CollectionNode assert(values.size() == elements_.size()); @@ -198,7 +230,7 @@ class CollectionStateData_ : public NodeStateData { updates_.clear(); if (older_checkpoint_ptr_ != nullptr) { - older_checkpoint_ptr_->emplace_updates(std::move(all_updates_)); + older_checkpoint_ptr_->commit_updates(std::move(all_updates_)); assert(all_updates_.empty()); } else { all_updates_.clear(); @@ -208,7 +240,9 @@ class CollectionStateData_ : public NodeStateData { } std::unique_ptr copy() const override { - return std::make_unique(*this); + auto uptr = std::make_unique(*this); + uptr->older_checkpoint_ptr_ = nullptr; // doesn't get to keep the checkpoints + return uptr; } std::span diff() const { return updates_; } @@ -240,16 +274,22 @@ class CollectionStateData_ : public NodeStateData { } void revert() { + updates_.clear(); + // Un-apply any changes by working backwards through all updates. // If we end up enforcing updates being sorted and unique later then // we could do this any order (or better in parallel). - for (const Update& update : all_updates_ | std::views::reverse) { elements_[update.index] = update.old; } - updates_.clear(); - all_updates_.clear(); + if (older_checkpoint_ptr_ != nullptr) { + older_checkpoint_ptr_->revert_updates(std::move(all_updates_)); + assert(all_updates_.empty()); + } else { + all_updates_.clear(); + } + size_ = previous_size_; } @@ -318,7 +358,7 @@ CollectionCheckpoint_::CollectionCheckpoint_(CollectionStateData_* state_ptr) : older_checkpoint_ptr_->newer_checkpoint_ptr_ = this; if (not state_ptr->all_updates_.empty()) { - older_checkpoint_ptr_->emplace_updates(state_ptr->all_updates_); // copy! + older_checkpoint_ptr_->commit_updates(state_ptr->all_updates_); // copy! } assert(older_checkpoint_ptr_->drop() == 0); } @@ -330,14 +370,19 @@ CollectionCheckpoint_::~CollectionCheckpoint_() { if (older_checkpoint_ptr_ == nullptr) { // We're the oldest checkpoint, so delete ourselves from the newer one // and let any information we're holding die with us - std::visit([](auto* ptr) { ptr->older_checkpoint_ptr_ = nullptr; }, newer_checkpoint_ptr_); + std::visit( + [](auto* ptr) { + if (ptr != nullptr) ptr->older_checkpoint_ptr_ = nullptr; + }, + newer_checkpoint_ptr_ + ); } else { // We're an intermediate checkpoint, so we need to pass any information we're // holding to the next oldest checkpoint and update the pointers on either side of // us assert(older_checkpoint_ptr_->drop_ == 0); for (std::vector& updates : updates_) { - older_checkpoint_ptr_->emplace_updates(std::move(updates)); + older_checkpoint_ptr_->commit_updates(std::move(updates)); } older_checkpoint_ptr_->drop_ = drop_; diff --git a/tests/cpp/nodes/test_collections.cpp b/tests/cpp/nodes/test_collections.cpp index 218d0cff..ed4818f6 100644 --- a/tests/cpp/nodes/test_collections.cpp +++ b/tests/cpp/nodes/test_collections.cpp @@ -12,11 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include -#include #include -#include +#include #include "dwave-optimization/graph.hpp" #include "dwave-optimization/nodes/collections.hpp" @@ -783,21 +783,68 @@ TEST_CASE("SetNode") { } } } + + AND_WHEN("We destruct the state before the checkpoint") { + state = graph.empty_state(); + checkpoint0.reset(); + } } WHEN("We mutate the state and then create a checkpoint before commiting") { set_ptr->assign(state, {4, 1}); auto checkpoint = set_ptr->checkpoint(state); - graph.propose(state); - CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); - set_ptr->assign(state, {3, 4, 1}); - graph.propose(state); - CHECK_THAT(set_ptr->view(state), RangeEquals({3, 4, 1})); + AND_WHEN("We do a sequence of commits") { + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); - set_ptr->assign_from_checkpoint(state, checkpoint); - graph.propose(state); - CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + set_ptr->assign(state, {3, 4, 1}); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({3, 4, 1})); + + set_ptr->assign_from_checkpoint(state, checkpoint); + graph.propose(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + } + + AND_WHEN("We revert and then restore from the checkpoint") { + graph.propagate(state); + graph.revert(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint)); + graph.propagate(state); + AND_WHEN("we commit the change to the checkpoint") { + graph.commit(state); + + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + } + } + + AND_WHEN("We make more changes, save another checkpoint and then revert") { + set_ptr->assign(state, {3, 2, 1, 0}); + auto checkpoint1 = set_ptr->checkpoint(state); + + graph.propagate(state); + graph.revert(state); + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + + AND_WHEN("We revert to the first checkpoint") { + checkpoint1.reset(); // need to get rid of the second checkpoint first + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint)); + graph.propose(state); + + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); + } + + AND_WHEN("We revert to the second checkpoint") { + set_ptr->assign_from_checkpoint(state, std::move(checkpoint1)); + graph.propose(state); + + CHECK_THAT(set_ptr->view(state), RangeEquals({3, 2, 1, 0})); + } + } } WHEN("We do several mutations and create several checkpoints within the same commit") { @@ -870,6 +917,82 @@ TEST_CASE("SetNode") { graph.propose(state); CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); } + + WHEN("We do some fuzzing with checkpoints") { + auto rng = std::default_random_engine(); + + // let's start by making a bunch of checkpoints with a copy of the buffer + // when the checkpoint was made + std::vector>> checkpoints; + { + // Randomly generate a state. There are more efficient ways + // to do this probably but for this test this is sufficient. + auto buffer = [&]() -> std::vector { + std::vector buff(5); + std::iota(buff.begin(), buff.end(), 0); + std::shuffle(buff.begin(), buff.end(), rng); + + std::uniform_int_distribution len(0, 4); + buff.erase(buff.begin() + len(rng), buff.end()); + + return buff; + }; + + // Commit anything that's pending + graph.propose(state); + + // Now, do a bunch of random actions + std::uniform_int_distribution action(0, 6); + for (int step = 0; step < 500; ++step) { + switch (action(rng)) { + case 0: + // make a checkpoint, tracking the current visible buffer + checkpoints.emplace_back( + set_ptr->checkpoint(state), + std::vector(set_ptr->begin(state), set_ptr->end(state)) + ); + break; + case 1: + // make a commit + graph.propagate(state); + graph.commit(state); + break; + case 2: + // make a revert + graph.propagate(state); + graph.revert(state); + break; + default: // we want to oversample this one + // assign a new state + set_ptr->assign(state, buffer()); + break; + } + } + + // Commit anything that's left over before the next step + graph.propose(state); + } + + // now, moving backwards through those checkpoints, let's randomly + // restore the state to the checkpoint or drop it + std::uniform_int_distribution flip(0, 1); + for (auto& [check, buff] : checkpoints | std::views::reverse) { + if (flip(rng)) { + set_ptr->assign_from_checkpoint(state, std::move(check)); + graph.propagate(state); + + CHECK_THAT(set_ptr->view(state), RangeEquals(buff)); + + if (flip(rng)) { + graph.commit(state); + } else { + graph.revert(state); + } + } else { + check.reset(); + } + } + } } } } From 9cd035b991321b3d67f390aabcca2df34247b414 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Mon, 20 Jul 2026 12:14:21 -0700 Subject: [PATCH 03/21] Use offical Python images in CircleCI This avoids using GCC11 which had some bugs in their ranges implementation. --- .circleci/config.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 08b2bc79..343d55d7 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -30,7 +30,7 @@ environment: &global-environment jobs: python-linux: docker: - - image: cimg/python:3.13 # just need a version that can install cibuildwheel + - image: cimg/python:3.13 # need a version that can install cibuildwheel and that has docker environment: <<: *global-environment @@ -57,7 +57,7 @@ jobs: python-linux-debug: docker: - - image: cimg/python:3.10 + - image: python:3.10 steps: - checkout @@ -127,7 +127,7 @@ jobs: python-sdist: docker: - - image: cimg/python:3.10 + - image: python:3.10 steps: - checkout @@ -239,7 +239,7 @@ jobs: serialization: docker: - - image: cimg/python:3.13 + - image: python:3.13 steps: - checkout @@ -282,7 +282,7 @@ jobs: docs: docker: - - image: cimg/python:3.13 # As of April 2026, the dwave-ocean-sdk uses 3.13 + - image: python:3.13 # As of April 2026, the dwave-ocean-sdk uses 3.13 steps: - checkout @@ -337,7 +337,7 @@ jobs: deploy: docker: - - image: cimg/python:3.10 + - image: python:3.10 steps: - attach_workspace: From 6ec49f916aff1150de913c046ac277f03c323b2c Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Mon, 20 Jul 2026 15:05:38 -0700 Subject: [PATCH 04/21] Add LinkedListCheckpoint helper class --- .../include/dwave-optimization/state.hpp | 4 +- dwave/optimization/src/nodes/_checkpoints.cpp | 44 +++++++ dwave/optimization/src/nodes/_checkpoints.hpp | 73 ++++++++++++ dwave/optimization/src/nodes/collections.cpp | 107 +++++------------- meson.build | 1 + 5 files changed, 151 insertions(+), 78 deletions(-) create mode 100644 dwave/optimization/src/nodes/_checkpoints.cpp create mode 100644 dwave/optimization/src/nodes/_checkpoints.hpp diff --git a/dwave/optimization/include/dwave-optimization/state.hpp b/dwave/optimization/include/dwave-optimization/state.hpp index e82b9cbc..a4a7e075 100644 --- a/dwave/optimization/include/dwave-optimization/state.hpp +++ b/dwave/optimization/include/dwave-optimization/state.hpp @@ -39,9 +39,9 @@ using State = typename std::vector>; /// A generic base class for node checkpoints. struct NodeStateCheckpoint { NodeStateCheckpoint() = default; - NodeStateCheckpoint(const NodeStateCheckpoint&) = default; + NodeStateCheckpoint(const NodeStateCheckpoint&) = delete; NodeStateCheckpoint(NodeStateCheckpoint&&) = delete; - NodeStateCheckpoint& operator=(const NodeStateCheckpoint&) = default; + NodeStateCheckpoint& operator=(const NodeStateCheckpoint&) = delete; NodeStateCheckpoint& operator=(NodeStateCheckpoint&&) = delete; virtual ~NodeStateCheckpoint() = default; diff --git a/dwave/optimization/src/nodes/_checkpoints.cpp b/dwave/optimization/src/nodes/_checkpoints.cpp new file mode 100644 index 00000000..77d2e95b --- /dev/null +++ b/dwave/optimization/src/nodes/_checkpoints.cpp @@ -0,0 +1,44 @@ +// Copyright 2026 D-Wave +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "_checkpoints.hpp" + +namespace dwave::optimization { + +// Place self between the state and any checkpoint it's currently holding +LinkedListCheckpoint::LinkedListCheckpoint(CheckpointableState& state) : + prev_ptr_(state.prev_ptr_), next_ptr_(&state) { + if (prev_ptr_ != nullptr) prev_ptr_->next_ptr_ = this; + state.prev_ptr_ = this; +} + +LinkedListCheckpoint::~LinkedListCheckpoint() { + if (prev_ptr_ != nullptr) prev_ptr_->next_ptr_ = next_ptr_; + + // Now make sure next_ptr is pointing to prev_ptr (which can be null) + std::visit( + [&](auto* next_ptr) -> void { + if (next_ptr == nullptr) return; // state was destructed first + next_ptr->prev_ptr_ = prev_ptr_; + }, + next_ptr_ + ); +} + +CheckpointableState::~CheckpointableState() { + if (prev_ptr_ == nullptr) return; // nothing to clean up + prev_ptr_->next_ptr_ = static_cast(nullptr); +} + +} // namespace dwave::optimization diff --git a/dwave/optimization/src/nodes/_checkpoints.hpp b/dwave/optimization/src/nodes/_checkpoints.hpp new file mode 100644 index 00000000..4f439daa --- /dev/null +++ b/dwave/optimization/src/nodes/_checkpoints.hpp @@ -0,0 +1,73 @@ +// Copyright 2026 D-Wave +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include + +#include "dwave-optimization/state.hpp" + +namespace dwave::optimization { + +class CheckpointableState; + +class LinkedListCheckpoint : public NodeStateCheckpoint { + public: + LinkedListCheckpoint() = delete; + // We're not moveable or copyable because NodeStateCheckpoint is not. + + LinkedListCheckpoint(CheckpointableState& state); + + ~LinkedListCheckpoint() override; + + protected: // todo: private? + friend CheckpointableState; + + LinkedListCheckpoint* prev_ptr_; + + // Is usually not nullptr unless the state has been destructed + std::variant next_ptr_; +}; + +class CheckpointableState : public NodeStateData { + public: + CheckpointableState() = default; + + CheckpointableState(const CheckpointableState& other) { + assert(false); + } + CheckpointableState(CheckpointableState&&) = default; + + CheckpointableState& operator=(const CheckpointableState&) { + assert(false); + } + CheckpointableState& operator=(CheckpointableState&&) = default; + + ~CheckpointableState(); + + protected: + template T> + T* checkpoint_ptr() { + return static_cast(prev_ptr_); + } + + private: // todo: private? + friend LinkedListCheckpoint; + + // The name is a bit confusing, but by making it match LinkedListCheckpoint::prev_ptr_ + // it makes the implementations of the various visit methods clearer. + LinkedListCheckpoint* prev_ptr_ = nullptr; // Will be nullptr if there are no checkpoints +}; + +} // namespace dwave::optimization diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index 0d168ab6..1d92bd49 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -18,7 +18,8 @@ #include #include #include -#include + +#include "_checkpoints.hpp" namespace dwave::optimization { @@ -70,13 +71,23 @@ std::vector augment_collection_(std::vector values, const ssize_ class CollectionStateData_; -class CollectionCheckpoint_ : public NodeStateCheckpoint { +class CollectionCheckpoint_ : public LinkedListCheckpoint { public: CollectionCheckpoint_() = delete; - CollectionCheckpoint_(CollectionStateData_* state_ptr); + CollectionCheckpoint_(CollectionStateData_& state); + + ~CollectionCheckpoint_() override { + // if we're the oldest checkpoint, just let whatever information we're + // holding get destructed with us + if (prev_ptr_ == nullptr) return; - ~CollectionCheckpoint_() override; + // otherwise we need to transfer our info over + auto* prev_ptr = static_cast(prev_ptr_); + assert(prev_ptr->drop_ == 0); + for (auto& updates : updates_) prev_ptr->commit_updates(std::move(updates)); + prev_ptr->drop_ = drop_; + } // detach the updates as a flattened view (in the forward order) auto detach_updates() { @@ -125,18 +136,13 @@ class CollectionCheckpoint_ : public NodeStateCheckpoint { bool valid() const override { return true; } private: - friend CollectionStateData_; - std::vector> updates_; ssize_t drop_; ssize_t size_; - - CollectionCheckpoint_* older_checkpoint_ptr_; - std::variant newer_checkpoint_ptr_; }; -class CollectionStateData_ : public NodeStateData { +class CollectionStateData_ : public CheckpointableState { public: explicit CollectionStateData_(ssize_t n) : CollectionStateData_(n, n) {} @@ -153,15 +159,6 @@ class CollectionStateData_ : public NodeStateData { assert(0 <= size_ and static_cast(size_) <= elements_.size()); } - ~CollectionStateData_() { - // make sure if we're destructed before the checkpoint that we clean - // up the dangling pointer - if (older_checkpoint_ptr_ != nullptr) { - older_checkpoint_ptr_->newer_checkpoint_ptr_ = - static_cast(nullptr); - } - } - void assign(std::vector&& values, ssize_t size) { // this should have been checked already by the CollectionNode assert(values.size() == elements_.size()); @@ -195,7 +192,7 @@ class CollectionStateData_ : public NodeStateData { // 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(older_checkpoint_ptr_ == checkpoint_ptr); + assert(this->checkpoint_ptr() == checkpoint_ptr); // Ok, let's get ourselves to the same place as the checkpoint @@ -223,14 +220,14 @@ class CollectionStateData_ : public NodeStateData { const double* buff() const { return elements_.data(); } std::unique_ptr checkpoint() { - return std::make_unique(this); + return std::make_unique(*this); } void commit() { updates_.clear(); - if (older_checkpoint_ptr_ != nullptr) { - older_checkpoint_ptr_->commit_updates(std::move(all_updates_)); + if (auto* checkpoint_ptr = this->checkpoint_ptr()) { + checkpoint_ptr->commit_updates(std::move(all_updates_)); assert(all_updates_.empty()); } else { all_updates_.clear(); @@ -239,12 +236,6 @@ class CollectionStateData_ : public NodeStateData { previous_size_ = size_; } - std::unique_ptr copy() const override { - auto uptr = std::make_unique(*this); - uptr->older_checkpoint_ptr_ = nullptr; // doesn't get to keep the checkpoints - return uptr; - } - std::span diff() const { return updates_; } void exchange(ssize_t i, ssize_t j) { @@ -283,8 +274,8 @@ class CollectionStateData_ : public NodeStateData { elements_[update.index] = update.old; } - if (older_checkpoint_ptr_ != nullptr) { - older_checkpoint_ptr_->revert_updates(std::move(all_updates_)); + if (auto* checkpoint_ptr = this->checkpoint_ptr()) { + checkpoint_ptr->revert_updates(std::move(all_updates_)); assert(all_updates_.empty()); } else { all_updates_.clear(); @@ -329,6 +320,8 @@ class CollectionStateData_ : public NodeStateData { ssize_t size_diff() const { return size_ - previous_size_; } private: + friend CollectionCheckpoint_; + // The elements in the collection std::vector elements_; @@ -343,54 +336,16 @@ class CollectionStateData_ : public NodeStateData { // commit/revert ssize_t size_; ssize_t previous_size_; - - friend CollectionCheckpoint_; - CollectionCheckpoint_* older_checkpoint_ptr_ = nullptr; }; -CollectionCheckpoint_::CollectionCheckpoint_(CollectionStateData_* state_ptr) : +CollectionCheckpoint_::CollectionCheckpoint_(CollectionStateData_& state) : + LinkedListCheckpoint(state), updates_(), - drop_(state_ptr->all_updates_.size()), // so we ignore any updates added before we're made - size_(state_ptr->size()), - older_checkpoint_ptr_(state_ptr->older_checkpoint_ptr_), - newer_checkpoint_ptr_(state_ptr) { - if (older_checkpoint_ptr_ != nullptr) { - older_checkpoint_ptr_->newer_checkpoint_ptr_ = this; - - if (not state_ptr->all_updates_.empty()) { - older_checkpoint_ptr_->commit_updates(state_ptr->all_updates_); // copy! - } - assert(older_checkpoint_ptr_->drop() == 0); - } - - std::get(newer_checkpoint_ptr_)->older_checkpoint_ptr_ = this; -} - -CollectionCheckpoint_::~CollectionCheckpoint_() { - if (older_checkpoint_ptr_ == nullptr) { - // We're the oldest checkpoint, so delete ourselves from the newer one - // and let any information we're holding die with us - std::visit( - [](auto* ptr) { - if (ptr != nullptr) ptr->older_checkpoint_ptr_ = nullptr; - }, - newer_checkpoint_ptr_ - ); - } else { - // We're an intermediate checkpoint, so we need to pass any information we're - // holding to the next oldest checkpoint and update the pointers on either side of - // us - assert(older_checkpoint_ptr_->drop_ == 0); - for (std::vector& updates : updates_) { - older_checkpoint_ptr_->commit_updates(std::move(updates)); - } - older_checkpoint_ptr_->drop_ = drop_; - - older_checkpoint_ptr_->newer_checkpoint_ptr_ = newer_checkpoint_ptr_; - std::visit( - [&](auto* ptr) { ptr->older_checkpoint_ptr_ = older_checkpoint_ptr_; }, - newer_checkpoint_ptr_ - ); + drop_(state.all_updates_.size()), // so we ignore any updates added before we're made + size_(state.size()) { + if (auto* prev_checkpoint = static_cast(prev_ptr_)) { + prev_checkpoint->commit_updates(state.all_updates_); + assert(prev_checkpoint->drop() == 0); } } diff --git a/meson.build b/meson.build index 690d7165..b8882ec8 100644 --- a/meson.build +++ b/meson.build @@ -27,6 +27,7 @@ py = import('python').find_installation(pure: false) dwave_optimization_include = include_directories('dwave/optimization/include/') dwave_optimization_src = [ + 'dwave/optimization/src/nodes/_checkpoints.cpp', 'dwave/optimization/src/nodes/binaryop.cpp', 'dwave/optimization/src/nodes/collections.cpp', 'dwave/optimization/src/nodes/constants.cpp', From fc44824cba614127daaf23e057324698e50d975e Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Mon, 20 Jul 2026 15:15:16 -0700 Subject: [PATCH 05/21] Fix the copying of checkpointed states --- dwave/optimization/src/nodes/_checkpoints.hpp | 8 ++------ dwave/optimization/src/nodes/collections.cpp | 4 ++++ tests/cpp/nodes/test_collections.cpp | 6 ++++++ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/dwave/optimization/src/nodes/_checkpoints.hpp b/dwave/optimization/src/nodes/_checkpoints.hpp index 4f439daa..3753b78b 100644 --- a/dwave/optimization/src/nodes/_checkpoints.hpp +++ b/dwave/optimization/src/nodes/_checkpoints.hpp @@ -44,14 +44,10 @@ class CheckpointableState : public NodeStateData { public: CheckpointableState() = default; - CheckpointableState(const CheckpointableState& other) { - assert(false); - } + CheckpointableState(const CheckpointableState&) {} // the checkpoint pointer is not copied CheckpointableState(CheckpointableState&&) = default; - CheckpointableState& operator=(const CheckpointableState&) { - assert(false); - } + CheckpointableState& operator=(const CheckpointableState&) = delete; CheckpointableState& operator=(CheckpointableState&&) = default; ~CheckpointableState(); diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index 1d92bd49..3f7e6c72 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -236,6 +236,10 @@ class CollectionStateData_ : public CheckpointableState { previous_size_ = size_; } + std::unique_ptr copy() const override { + return std::make_unique(*this); + } + std::span diff() const { return updates_; } void exchange(ssize_t i, ssize_t j) { diff --git a/tests/cpp/nodes/test_collections.cpp b/tests/cpp/nodes/test_collections.cpp index ed4818f6..c8ac10d0 100644 --- a/tests/cpp/nodes/test_collections.cpp +++ b/tests/cpp/nodes/test_collections.cpp @@ -788,6 +788,12 @@ TEST_CASE("SetNode") { state = graph.empty_state(); checkpoint0.reset(); } + + THEN("We can copy the state") { + auto cp = state[0]->copy(); + // this is a smoke test because there is no public way to check + // that the checkpoint didn't get copied over + } } WHEN("We mutate the state and then create a checkpoint before commiting") { From f711e63900c1cc1a5ae897011928d6cef448a85e Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Mon, 20 Jul 2026 15:25:51 -0700 Subject: [PATCH 06/21] Drop unused NodeStateCheckpoint::valid() method --- dwave/optimization/include/dwave-optimization/state.hpp | 3 --- dwave/optimization/src/nodes/collections.cpp | 2 -- 2 files changed, 5 deletions(-) diff --git a/dwave/optimization/include/dwave-optimization/state.hpp b/dwave/optimization/include/dwave-optimization/state.hpp index a4a7e075..4b270db9 100644 --- a/dwave/optimization/include/dwave-optimization/state.hpp +++ b/dwave/optimization/include/dwave-optimization/state.hpp @@ -45,9 +45,6 @@ struct NodeStateCheckpoint { NodeStateCheckpoint& operator=(NodeStateCheckpoint&&) = delete; virtual ~NodeStateCheckpoint() = default; - - /// Whether the checkpoint is still available to be used. - virtual bool valid() const = 0; }; using checkpoint_type = std::unique_ptr; diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index 3f7e6c72..79f2afd6 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -133,8 +133,6 @@ class CollectionCheckpoint_ : public LinkedListCheckpoint { ssize_t size() { return size_; } - bool valid() const override { return true; } - private: std::vector> updates_; ssize_t drop_; From e85f85f2339c3c3a82810b44c2734c9e62fbf98c Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Tue, 21 Jul 2026 13:01:50 -0700 Subject: [PATCH 07/21] Add DiffCheckpoint class for checkpoints that track diffs --- dwave/optimization/src/nodes/_checkpoints.cpp | 52 ++++++++++++- dwave/optimization/src/nodes/_checkpoints.hpp | 62 ++++++++++----- dwave/optimization/src/nodes/_state.hpp | 16 ++++ dwave/optimization/src/nodes/collections.cpp | 75 +------------------ 4 files changed, 113 insertions(+), 92 deletions(-) diff --git a/dwave/optimization/src/nodes/_checkpoints.cpp b/dwave/optimization/src/nodes/_checkpoints.cpp index 77d2e95b..60a4d8bc 100644 --- a/dwave/optimization/src/nodes/_checkpoints.cpp +++ b/dwave/optimization/src/nodes/_checkpoints.cpp @@ -16,6 +16,11 @@ namespace dwave::optimization { +CheckpointableState::~CheckpointableState() { + if (prev_ptr_ == nullptr) return; // nothing to clean up + prev_ptr_->next_ptr_ = static_cast(nullptr); +} + // Place self between the state and any checkpoint it's currently holding LinkedListCheckpoint::LinkedListCheckpoint(CheckpointableState& state) : prev_ptr_(state.prev_ptr_), next_ptr_(&state) { @@ -36,9 +41,50 @@ LinkedListCheckpoint::~LinkedListCheckpoint() { ); } -CheckpointableState::~CheckpointableState() { - if (prev_ptr_ == nullptr) return; // nothing to clean up - prev_ptr_->next_ptr_ = static_cast(nullptr); +DiffCheckpoint::DiffCheckpoint(CheckpointableState& state, std::span diff) : + LinkedListCheckpoint(state), updates_(), drop_(diff.size()) { + if (auto* prev_ptr = static_cast(prev_ptr_)) { + prev_ptr->commit_updates(std::vector(diff.begin(), diff.end())); + assert(prev_ptr->drop_ == 0); + } +} + +DiffCheckpoint::~DiffCheckpoint() { + // if we're the oldest checkpoint, just let whatever information we're + // holding get destructed with us + if (prev_ptr_ == nullptr) return; + + // otherwise we need to transfer our info over + auto* prev_ptr = static_cast(prev_ptr_); + assert(prev_ptr->drop_ == 0); + for (auto& updates : updates_) prev_ptr->commit_updates(std::move(updates)); + prev_ptr->drop_ = drop_; +} + +void DiffCheckpoint::commit_updates(std::vector updates) { + assert(0 <= drop_ and static_cast(drop_) <= updates.size()); + + if (drop_) { + updates.erase(updates.begin(), updates.begin() + drop_); + drop_ = 0; + } + + updates_.emplace_back(std::move(updates)); +} + +void DiffCheckpoint::revert_updates(std::vector updates) { + assert(0 <= drop_ and static_cast(drop_) <= updates.size()); + + if (not drop_) return; // nothing to do + + // We want to track the updates that would revert the changes from the + // current state. + // In C++23 we could use assign_range() which would be nicer + auto relevant = std::move(updates) | std::views::take(drop_) | std::views::reverse | + std::views::transform([](const Update& up) { return up.inverse(); }); + updates_.emplace_back(relevant.begin(), relevant.end()); + + drop_ = 0; } } // namespace dwave::optimization diff --git a/dwave/optimization/src/nodes/_checkpoints.hpp b/dwave/optimization/src/nodes/_checkpoints.hpp index 3753b78b..39ac72db 100644 --- a/dwave/optimization/src/nodes/_checkpoints.hpp +++ b/dwave/optimization/src/nodes/_checkpoints.hpp @@ -14,13 +14,42 @@ #pragma once +#include #include +#include +#include "dwave-optimization/array.hpp" #include "dwave-optimization/state.hpp" namespace dwave::optimization { -class CheckpointableState; +class LinkedListCheckpoint; + +class CheckpointableState { + public: + CheckpointableState() = default; + + CheckpointableState(const CheckpointableState&) {} // the checkpoint pointer is not copied + CheckpointableState(CheckpointableState&&) = default; + + CheckpointableState& operator=(const CheckpointableState&) = delete; + CheckpointableState& operator=(CheckpointableState&&) = default; + + ~CheckpointableState(); + + protected: + template T> + T* checkpoint_ptr() { + return static_cast(prev_ptr_); + } + + private: // todo: private? + friend LinkedListCheckpoint; + + // The name is a bit confusing, but by making it match LinkedListCheckpoint::prev_ptr_ + // it makes the implementations of the various visit methods clearer. + LinkedListCheckpoint* prev_ptr_ = nullptr; // Will be nullptr if there are no checkpoints +}; class LinkedListCheckpoint : public NodeStateCheckpoint { public: @@ -40,30 +69,27 @@ class LinkedListCheckpoint : public NodeStateCheckpoint { std::variant next_ptr_; }; -class CheckpointableState : public NodeStateData { +class DiffCheckpoint : public LinkedListCheckpoint { public: - CheckpointableState() = default; + DiffCheckpoint(CheckpointableState& state, std::span diff); - CheckpointableState(const CheckpointableState&) {} // the checkpoint pointer is not copied - CheckpointableState(CheckpointableState&&) = default; + ~DiffCheckpoint() override; - CheckpointableState& operator=(const CheckpointableState&) = delete; - CheckpointableState& operator=(CheckpointableState&&) = default; - - ~CheckpointableState(); + void commit_updates(std::vector updates); - protected: - template T> - T* checkpoint_ptr() { - return static_cast(prev_ptr_); + auto detach_updates() { + auto updates = std::move(updates_) | std::views::join; + assert(updates_.empty()); + return updates; } - private: // todo: private? - friend LinkedListCheckpoint; + ssize_t& drop() { return drop_; } - // The name is a bit confusing, but by making it match LinkedListCheckpoint::prev_ptr_ - // it makes the implementations of the various visit methods clearer. - LinkedListCheckpoint* prev_ptr_ = nullptr; // Will be nullptr if there are no checkpoints + void revert_updates(std::vector updates); + + private: + std::vector> updates_; + ssize_t drop_; }; } // namespace dwave::optimization diff --git a/dwave/optimization/src/nodes/_state.hpp b/dwave/optimization/src/nodes/_state.hpp index bc715202..5f4c8162 100644 --- a/dwave/optimization/src/nodes/_state.hpp +++ b/dwave/optimization/src/nodes/_state.hpp @@ -91,6 +91,22 @@ class ArrayStateData { assert(size_ >= 0 && static_cast(size_) == buffer.size()); } + // Commit the changes and clear the diff by returning the diff buffer. + std::vector commit_and_detach() { + std::vector tmp; + std::swap(updates, tmp); + // AlexC: we could now do updates.reserve(tmp.size()) under the assumption + // that future update buffers will be a similar size. On the other hand, + // not doing this provides another meaningful difference to ::commit(). + // For now, I think it make sense to not but performance testing needed. + + previous_size_ = buffer.size(); + assert(size_ >= 0 && static_cast(size_) == buffer.size()); + + assert(updates.empty()); + return tmp; + } + std::span diff() const noexcept { return updates; } // Append a new value to the buffer, tracking the addition in the diff diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index 79f2afd6..fb265ed6 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -71,76 +71,17 @@ std::vector augment_collection_(std::vector values, const ssize_ class CollectionStateData_; -class CollectionCheckpoint_ : public LinkedListCheckpoint { +class CollectionCheckpoint_ : public DiffCheckpoint { public: - CollectionCheckpoint_() = delete; - CollectionCheckpoint_(CollectionStateData_& state); - ~CollectionCheckpoint_() override { - // if we're the oldest checkpoint, just let whatever information we're - // holding get destructed with us - if (prev_ptr_ == nullptr) return; - - // otherwise we need to transfer our info over - auto* prev_ptr = static_cast(prev_ptr_); - assert(prev_ptr->drop_ == 0); - for (auto& updates : updates_) prev_ptr->commit_updates(std::move(updates)); - prev_ptr->drop_ = drop_; - } - - // detach the updates as a flattened view (in the forward order) - auto detach_updates() { - auto updates = std::move(updates_) | std::views::join; - assert(updates_.empty()); - return updates; - } - - ssize_t& drop() { return drop_; } - ssize_t drop() const { return drop_; } - - // Track the updates associated with a commit - void commit_updates(std::vector updates) { - assert(0 <= drop_ and static_cast(drop_) <= updates.size()); - - if (not drop_) { - updates_.emplace_back(std::move(updates)); - return; - } - - // Otherwise we only want to take the updates up to drop - // In C++23 we could use assign_range() which would be nicer - auto relevant = std::move(updates) | std::views::drop(drop_); - updates_.emplace_back(relevant.begin(), relevant.end()); - drop_ = 0; - } - - // Track the updates associated with a revert - void revert_updates(std::vector updates) { - assert(0 <= drop_ and static_cast(drop_) <= updates.size()); - - if (not drop_) return; // nothing to do - - // We want to track the updates that would revert the changes from the - // current state. - // In C++23 we could use assign_range() which would be nicer - auto relevant = std::move(updates) | std::views::take(drop_) | std::views::reverse | - std::views::transform([](const Update& up) { return up.inverse(); }); - updates_.emplace_back(relevant.begin(), relevant.end()); - - drop_ = 0; - } - - ssize_t size() { return size_; } + ssize_t size() const { return size_; } private: - std::vector> updates_; - ssize_t drop_; - ssize_t size_; }; -class CollectionStateData_ : public CheckpointableState { +class CollectionStateData_ : public NodeStateData, public CheckpointableState { public: explicit CollectionStateData_(ssize_t n) : CollectionStateData_(n, n) {} @@ -341,15 +282,7 @@ class CollectionStateData_ : public CheckpointableState { }; CollectionCheckpoint_::CollectionCheckpoint_(CollectionStateData_& state) : - LinkedListCheckpoint(state), - updates_(), - drop_(state.all_updates_.size()), // so we ignore any updates added before we're made - size_(state.size()) { - if (auto* prev_checkpoint = static_cast(prev_ptr_)) { - prev_checkpoint->commit_updates(state.all_updates_); - assert(prev_checkpoint->drop() == 0); - } -} + DiffCheckpoint(state, state.all_updates_), size_(state.size()) {} CollectionNode::CollectionNode(ssize_t max_value, ssize_t min_size, ssize_t max_size) : ArrayOutputMixin((min_size == max_size) ? max_size : Array::DYNAMIC_SIZE), From f70699e837c57a82d1b435c1e2489abbda66f4a1 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Tue, 21 Jul 2026 14:16:23 -0700 Subject: [PATCH 08/21] Go back to using CircleCI image for some CI tasks --- .circleci/config.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 343d55d7..b66c34b2 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -239,7 +239,7 @@ jobs: serialization: docker: - - image: python:3.13 + - image: cimg/python:3.13 steps: - checkout @@ -282,7 +282,7 @@ jobs: docs: docker: - - image: python:3.13 # As of April 2026, the dwave-ocean-sdk uses 3.13 + - image: cimg/python:3.13 # As of April 2026, the dwave-ocean-sdk uses 3.13 steps: - checkout @@ -337,7 +337,7 @@ jobs: deploy: docker: - - image: python:3.10 + - image: cimg/python:3.10 steps: - attach_workspace: From ef950e42b4c04022c4a1061e815b6e2aba30a1fd Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Thu, 23 Jul 2026 15:28:45 -0700 Subject: [PATCH 09/21] Add NumberNode::checkpoint() --- .../dwave-optimization/nodes/numbers.hpp | 7 +- dwave/optimization/src/nodes/_checkpoints.cpp | 3 + dwave/optimization/src/nodes/_checkpoints.hpp | 3 + dwave/optimization/src/nodes/_state.hpp | 23 +++ dwave/optimization/src/nodes/numbers.cpp | 136 +++++++++++++++- tests/cpp/nodes/test_collections.cpp | 2 + tests/cpp/nodes/test_numbers.cpp | 149 +++++++++++++++++- 7 files changed, 313 insertions(+), 10 deletions(-) diff --git a/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp b/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp index 252e709f..34cc9069 100644 --- a/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp +++ b/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp @@ -17,7 +17,6 @@ #include #include #include -#include #include #include "dwave-optimization/array.hpp" @@ -122,6 +121,8 @@ class NumberNode : public ArrayOutputMixin, public DecisionNode { // NumberNode methods ***************************************************** + std::unique_ptr checkpoint(State& state) const; + // In the given state, swap the value of index i with the value of index j. // Users may pass the slices (per sum constraint) that each index lies on. void exchange( @@ -290,6 +291,10 @@ class IntegerNode : public NumberNode { // IntegerNode methods **************************************************** + /// Set the current state to match the one at the time the given checkpoint was created. + void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const; + void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const; + // Set the value at the given index in the given state. // Users may pass the slices (per sum constraint) that each index lies on. void set_value( diff --git a/dwave/optimization/src/nodes/_checkpoints.cpp b/dwave/optimization/src/nodes/_checkpoints.cpp index 60a4d8bc..4b3fe59b 100644 --- a/dwave/optimization/src/nodes/_checkpoints.cpp +++ b/dwave/optimization/src/nodes/_checkpoints.cpp @@ -41,6 +41,9 @@ LinkedListCheckpoint::~LinkedListCheckpoint() { ); } +DiffCheckpoint::DiffCheckpoint(CheckpointableState& state, ssize_t drop) : + LinkedListCheckpoint(state), updates_(), drop_(drop) {} + DiffCheckpoint::DiffCheckpoint(CheckpointableState& state, std::span diff) : LinkedListCheckpoint(state), updates_(), drop_(diff.size()) { if (auto* prev_ptr = static_cast(prev_ptr_)) { diff --git a/dwave/optimization/src/nodes/_checkpoints.hpp b/dwave/optimization/src/nodes/_checkpoints.hpp index 39ac72db..8e1866d7 100644 --- a/dwave/optimization/src/nodes/_checkpoints.hpp +++ b/dwave/optimization/src/nodes/_checkpoints.hpp @@ -87,6 +87,9 @@ class DiffCheckpoint : public LinkedListCheckpoint { void revert_updates(std::vector updates); + protected: + DiffCheckpoint(CheckpointableState& state, ssize_t drop); + private: std::vector> updates_; ssize_t drop_; diff --git a/dwave/optimization/src/nodes/_state.hpp b/dwave/optimization/src/nodes/_state.hpp index 5f4c8162..b5e4d812 100644 --- a/dwave/optimization/src/nodes/_state.hpp +++ b/dwave/optimization/src/nodes/_state.hpp @@ -165,6 +165,29 @@ class ArrayStateData { size_ = buffer.size(); } + // Commit the changes and clear the diff by returning the diff buffer. + std::vector revert_and_detach() { + assert(previous_size_ >= 0); + buffer.resize(previous_size_); + const ssize_t size = buffer.size(); + for (const auto& [index, old, _] : updates | std::views::reverse) { + assert(index >= 0); + if (index >= size) continue; + buffer[index] = old; + } + size_ = buffer.size(); + + std::vector tmp; + std::swap(updates, tmp); + // AlexC: we could now do updates.reserve(tmp.size()) under the assumption + // that future update buffers will be a similar size. On the other hand, + // not doing this provides another meaningful difference to ::commit(). + // For now, I think it make sense to not but performance testing needed. + + assert(updates.empty()); + return tmp; + } + // Set the value at index, tracking the change in the diff. // If allow_emplace is true, do an emplace_back iff the index is equal to the current size. bool set(ssize_t i, double value, bool allow_emplace = false) { diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index 317082ed..30920d53 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -23,6 +23,7 @@ #include #include +#include "_checkpoints.hpp" #include "_state.hpp" #include "dwave-optimization/array.hpp" #include "dwave-optimization/common.hpp" @@ -72,8 +73,75 @@ NumberNode::SumConstraint::Operator NumberNode::SumConstraint::op(const ssize_t return operators_[slice]; } +class NumberNodeCheckpoint_ : public DiffCheckpoint { + public: + using slice_cache_type = std::vector>; + + NumberNodeCheckpoint_( + CheckpointableState& state, + std::span diff, + const slice_cache_type& slice_cache + ) : + DiffCheckpoint(state, diff.size()), slice_caches_() { + // If there is an older checkpoint, we want to put anything we're currently + // holding in our slice cache onto it + if (auto* prev_ptr = static_cast(prev_ptr_)) { + prev_ptr->commit_updates(std::vector(diff.begin(), diff.end()), slice_cache); + assert(prev_ptr->drop() == 0); + } + } + + void commit_updates(std::vector updates, slice_cache_type slice_cache) { + ssize_t drop = this->drop(); + assert(0 <= drop and static_cast(drop) <= updates.size()); + + if (not slice_cache.empty()) { + assert(updates.size() == slice_cache.size()); + + if (drop) { + slice_cache.erase(slice_cache.begin(), slice_cache.begin() + drop); + } + slice_caches_.emplace_back(std::move(slice_cache)); + } + + DiffCheckpoint::commit_updates(std::move(updates)); + assert(this->drop() == 0); + } + + auto detach_slice_cache() { + using join_type = decltype(std::move(slice_caches_) | std::views::join); + + if (slice_caches_.empty()) return std::optional(); + + auto joined = std::move(slice_caches_) | std::views::join; + assert(slice_caches_.empty()); + return std::optional(std::move(joined)); + } + + void revert_updates(std::vector updates, slice_cache_type slice_cache) { + ssize_t drop = this->drop(); + assert(0 <= drop and static_cast(drop) <= updates.size()); + + if (not slice_cache.empty()) { + assert(updates.size() == slice_cache.size()); + + if (drop) { + slice_cache.erase(slice_cache.begin() + drop, slice_cache.end()); + } + std::reverse(slice_cache.begin(), slice_cache.end()); + slice_caches_.emplace_back(std::move(slice_cache)); + } + + DiffCheckpoint::revert_updates(std::move(updates)); + assert(this->drop() == 0); + } + + private: + std::vector slice_caches_; +}; + /// State dependent data attached to NumberNode -struct NumberNodeStateData : public ArrayNodeStateData { +class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableState { public: // User does not provide sum constraints. NumberNodeStateData(std::vector input) : ArrayNodeStateData(std::move(input)) {} @@ -84,14 +152,28 @@ struct NumberNodeStateData : public ArrayNodeStateData { ) : ArrayNodeStateData(std::move(input)), sum_constraints_lhs(std::move(sum_constraints_lhs)) {} + std::unique_ptr checkpoint() { + return std::make_unique(*this, this->diff(), this->slice_cache_); + } + std::unique_ptr copy() const override { return std::make_unique(*this); } /// Commit the state dependent data of NumberNode. void commit() { - ArrayNodeStateData::commit(); // Commit changes to the buffer. - slice_cache_.clear(); // Empty the slice cache. + if (auto* checkpoint_ptr = this->checkpoint_ptr()) { + checkpoint_ptr->commit_updates( + ArrayNodeStateData::commit_and_detach(), std::move(slice_cache_) + ); + } else { + ArrayNodeStateData::commit(); // Commit changes to the buffer. + slice_cache_.clear(); // Empty the slice cache. + } + + // everything should have been cleared out regardless of which path we took + assert(this->diff().empty()); + assert(slice_cache_.empty()); } /// Revert the state dependent data of NumberNode. @@ -142,10 +224,16 @@ void NumberNodeStateData::revert() { sum_constraints_lhs[j][slices[j]] -= difference; } } - slice_cache_.clear(); // Empty the slice cache. } - ArrayNodeStateData::revert(); // Revert changes to the buffer. + if (auto* checkpoint_ptr = this->checkpoint_ptr()) { + checkpoint_ptr->revert_updates( + ArrayNodeStateData::revert_and_detach(), std::move(slice_cache_) + ); + } else { + slice_cache_.clear(); // Empty the slice cache. + ArrayNodeStateData::revert(); // Revert changes to the buffer. + } } void NumberNodeStateData::update( @@ -529,6 +617,10 @@ void NumberNode::propagate(State& state) const { } } +std::unique_ptr NumberNode::checkpoint(State& state) const { + return data_ptr_(state)->checkpoint(); +} + void NumberNode::commit(State& state) const noexcept { data_ptr_(state)->commit(); } @@ -953,6 +1045,40 @@ IntegerNode::IntegerNode( std::move(sum_constraints) ) {} +void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const { + auto state_data = data_ptr_(state); + + auto* checkpoint_ptr = static_cast(checkpoint.get()); + + // todo: assert that this checkpoint is the latest + + auto updates = checkpoint_ptr->detach_updates(); + auto slice_cache = checkpoint_ptr->detach_slice_cache(); // this is an std::optional<...>! + + if (slice_cache.has_value()) { + assert(sum_constraints_.size() > 0); + + auto slices_rit = std::ranges::rbegin(*slice_cache); + + for (const auto& [idx, old, _] : std::move(updates) | std::views::reverse) { + state_data->set(idx, old); + state_data->update(*this, idx, old - diff(state).back().old, *(slices_rit++)); + } + } else { + assert(updates.empty() or sum_constraints_.empty()); + + // in this case we don't need to do anything to update the slice data + for (const auto& [idx, old, _] : std::move(updates) | std::views::reverse) { + state_data->set(idx, old); + } + } +} + +void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const { + assign_from_checkpoint(state, checkpoint); // call the lvalue version + checkpoint.reset(); +} + bool IntegerNode::integral() const { return true; } bool IntegerNode::is_valid(ssize_t index, double value) const { diff --git a/tests/cpp/nodes/test_collections.cpp b/tests/cpp/nodes/test_collections.cpp index c8ac10d0..b7820e5f 100644 --- a/tests/cpp/nodes/test_collections.cpp +++ b/tests/cpp/nodes/test_collections.cpp @@ -873,6 +873,8 @@ TEST_CASE("SetNode") { graph.propose(state); THEN("we can go backwards through them without commiting and everything is correct") { + // TODO: check mutating before assigning + set_ptr->assign_from_checkpoint(state, std::move(checkpoint4)); CHECK_THAT(set_ptr->view(state), RangeEquals({2})); diff --git a/tests/cpp/nodes/test_numbers.cpp b/tests/cpp/nodes/test_numbers.cpp index 4349961d..0e10ae0d 100644 --- a/tests/cpp/nodes/test_numbers.cpp +++ b/tests/cpp/nodes/test_numbers.cpp @@ -23,6 +23,7 @@ #include "dwave-optimization/graph.hpp" #include "dwave-optimization/nodes/numbers.hpp" +#include "dwave-optimization/nodes/testing.hpp" using Catch::Matchers::RangeEquals; @@ -1892,6 +1893,8 @@ TEST_CASE("IntegerNode") { GIVEN("An Integer Node representing an 1d array of 10 elements with lower bound -10") { auto ptr = graph.emplace_node(std::initializer_list{10}, -10); + graph.emplace_node(ptr); + THEN("The shape is fixed") { CHECK(ptr->ndim() == 1); CHECK(ptr->size() == 10); @@ -1985,6 +1988,46 @@ TEST_CASE("IntegerNode") { } } } + + AND_WHEN("We checkpoint the state and then mutate") { + auto checkpoint = ptr->checkpoint(state); // [-4, -4, -2, -2, 0, 0, 2, 2, 4, 4] + + ptr->exchange(state, 0, 2); // [-2, -4, -4, -2, 0, 0, 2, 2, 4, 4] + ptr->set_value(state, 3, 1); // [-2, -4, -4, 1, 0, 0, 2, 2, 4, 4] + + THEN("We can commit, then assign from the checkpoint") { + graph.propose(state); + + ptr->assign_from_checkpoint(state, checkpoint); + + CHECK_THAT(ptr->view(state), RangeEquals({-4, -4, -2, -2, 0, 0, 2, 2, 4, 4})); + } + } + + AND_WHEN("We mutate, checkpoint the state, and then mutate again") { + ptr->set_value(state, 3, 1); // [-4, -4, -2, 1, 0, 0, 2, 2, 4, 4] + + auto checkpoint = ptr->checkpoint(state); + + ptr->exchange(state, 0, 2); // [-2, -4, -4, 1, 0, 0, 2, 2, 4, 4] + + THEN("We can commit, then assign from the checkpoint") { + graph.propose(state); + + ptr->assign_from_checkpoint(state, checkpoint); + + CHECK_THAT(ptr->view(state), RangeEquals({-4, -4, -2, 1, 0, 0, 2, 2, 4, 4})); + } + + THEN("We can revert, then assign from the checkpoint") { + graph.propagate(state); + graph.revert(state); + + ptr->assign_from_checkpoint(state, checkpoint); + + CHECK_THAT(ptr->view(state), RangeEquals({-4, -4, -2, 1, 0, 0, 2, 2, 4, 4})); + } + } } } @@ -2195,6 +2238,8 @@ TEST_CASE("IntegerNode") { std::initializer_list{2, 2, 2}, -5, 8, sum_constraints ); + graph.emplace_node(inode_ptr); + THEN("Sum constraint is correct") { CHECK(inode_ptr->sum_constraints().size() == 1); SumConstraint inode_sum_constraint = inode_ptr->sum_constraints()[0]; @@ -2209,14 +2254,110 @@ TEST_CASE("IntegerNode") { auto state = graph.initialize_state(); graph.initialize_state(state); std::vector expected_init{8, 8, 8, 8, 8, 8, -3, -5}; - auto sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); THEN("Sum constraint sums and state are correct") { - CHECK(inode_ptr->sum_constraints_lhs(state).size() == 1); - CHECK(inode_ptr->sum_constraints_lhs(state).data()[0].size() == 1); - CHECK_THAT(inode_ptr->sum_constraints_lhs(state)[0], RangeEquals({40})); + auto sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); + + CHECK(sum_constraints_lhs.size() == 1); + CHECK(sum_constraints_lhs.data()[0].size() == 1); + CHECK_THAT(sum_constraints_lhs[0], RangeEquals({40})); CHECK_THAT(inode_ptr->view(state), RangeEquals(expected_init)); } + + AND_WHEN("We create a checkpoint and then mutate the state") { + auto checkpoint = inode_ptr->checkpoint(state); + + inode_ptr->set_value(state, 7, 3); // [ 8, 8, 8, 8, 8, 8, -3, 3 ] + inode_ptr->exchange(state, 1, 6); // [ 8, -3, 8, 8, 8, 8, 8, 3 ] + + THEN("After committing, We can revert to that checkpoint") { + graph.propose(state); + inode_ptr->assign_from_checkpoint(state, std::move(checkpoint)); + + auto sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); + + CHECK(sum_constraints_lhs.size() == 1); + CHECK(sum_constraints_lhs.data()[0].size() == 1); + CHECK_THAT(sum_constraints_lhs[0], RangeEquals({40})); + CHECK_THAT(inode_ptr->view(state), RangeEquals(expected_init)); + } + } + + AND_WHEN("We mutate, create a checkpoint, and then mutate some more") { + inode_ptr->set_value(state, 7, 3); // [ 8, 8, 8, 8, 8, 8, -3, 3 ] + auto checkpoint = inode_ptr->checkpoint(state); + inode_ptr->exchange(state, 1, 6); // [ 8, -3, 8, 8, 8, 8, 8, 3 ] + + THEN("After committing, we can assign from that checkpoint") { + graph.propose(state); + inode_ptr->assign_from_checkpoint(state, std::move(checkpoint)); + + auto sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); + CHECK(sum_constraints_lhs.size() == 1); + CHECK(sum_constraints_lhs.data()[0].size() == 1); + CHECK_THAT(sum_constraints_lhs[0], RangeEquals({48})); + CHECK_THAT(inode_ptr->view(state), RangeEquals({8, 8, 8, 8, 8, 8, -3, 3})); + } + + THEN("After reverting, we can assign from that checkpoint") { + graph.propagate(state); + graph.revert(state); + inode_ptr->assign_from_checkpoint(state, std::move(checkpoint)); + + auto sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); + CHECK(sum_constraints_lhs.size() == 1); + CHECK(sum_constraints_lhs.data()[0].size() == 1); + CHECK_THAT(sum_constraints_lhs[0], RangeEquals({48})); + CHECK_THAT(inode_ptr->view(state), RangeEquals({8, 8, 8, 8, 8, 8, -3, 3})); + } + + AND_WHEN("We create a new checkpoint") { + auto checkpoint1 = inode_ptr->checkpoint(state); + + THEN("We can commit, and restore the checkpoints") { + inode_ptr->exchange(state, 1, 2); // [ 8, 8, -3, 8, 8, 8, 8, 3 ] + graph.propose(state); + + inode_ptr->assign_from_checkpoint(state, std::move(checkpoint1)); + + auto sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); + CHECK(sum_constraints_lhs.size() == 1); + CHECK(sum_constraints_lhs.data()[0].size() == 1); + CHECK_THAT(sum_constraints_lhs[0], RangeEquals({48})); + CHECK_THAT(inode_ptr->view(state), RangeEquals({8, -3, 8, 8, 8, 8, 8, 3})); + + inode_ptr->assign_from_checkpoint(state, std::move(checkpoint)); + + sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); + CHECK(sum_constraints_lhs.size() == 1); + CHECK(sum_constraints_lhs.data()[0].size() == 1); + CHECK_THAT(sum_constraints_lhs[0], RangeEquals({48})); + CHECK_THAT(inode_ptr->view(state), RangeEquals({8, 8, 8, 8, 8, 8, -3, 3})); + } + + THEN("We can revert, and restore the checkpoints") { + inode_ptr->exchange(state, 1, 2); // [ 8, 8, -3, 8, 8, 8, 8, 3 ] + graph.propagate(state); + graph.revert(state); + + inode_ptr->assign_from_checkpoint(state, std::move(checkpoint1)); + + auto sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); + CHECK(sum_constraints_lhs.size() == 1); + CHECK(sum_constraints_lhs.data()[0].size() == 1); + CHECK_THAT(sum_constraints_lhs[0], RangeEquals({48})); + CHECK_THAT(inode_ptr->view(state), RangeEquals({8, -3, 8, 8, 8, 8, 8, 3})); + + inode_ptr->assign_from_checkpoint(state, std::move(checkpoint)); + + sum_constraints_lhs = inode_ptr->sum_constraints_lhs(state); + CHECK(sum_constraints_lhs.size() == 1); + CHECK(sum_constraints_lhs.data()[0].size() == 1); + CHECK_THAT(sum_constraints_lhs[0], RangeEquals({48})); + CHECK_THAT(inode_ptr->view(state), RangeEquals({8, 8, 8, 8, 8, 8, -3, 3})); + } + } + } } } From e61a70f7e041e31540b6ddd4994e29ad3d418508 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Mon, 27 Jul 2026 09:05:41 -0700 Subject: [PATCH 10/21] Fix assigning from a checkpoint after mutation --- dwave/optimization/src/nodes/_state.hpp | 2 +- dwave/optimization/src/nodes/collections.cpp | 19 +++++++++++ dwave/optimization/src/nodes/numbers.cpp | 34 ++++++++++++++++---- tests/cpp/nodes/test_collections.cpp | 24 ++++++++++++-- tests/cpp/nodes/test_numbers.cpp | 9 ++++++ 5 files changed, 78 insertions(+), 10 deletions(-) diff --git a/dwave/optimization/src/nodes/_state.hpp b/dwave/optimization/src/nodes/_state.hpp index b5e4d812..5a2da6cb 100644 --- a/dwave/optimization/src/nodes/_state.hpp +++ b/dwave/optimization/src/nodes/_state.hpp @@ -181,7 +181,7 @@ class ArrayStateData { std::swap(updates, tmp); // AlexC: we could now do updates.reserve(tmp.size()) under the assumption // that future update buffers will be a similar size. On the other hand, - // not doing this provides another meaningful difference to ::commit(). + // not doing this provides another meaningful difference to ::revert(). // For now, I think it make sense to not but performance testing needed. assert(updates.empty()); diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index fb265ed6..a08f3a09 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -133,6 +133,25 @@ class CollectionStateData_ : public NodeStateData, public CheckpointableState { // we need right now. assert(this->checkpoint_ptr() == checkpoint_ptr); + // Check if there are any changes not otherwise tracked by a checkpoint that we need + // to revert first. + // A better way would be to implement a partial revert on our state class, but this + // is not a path we care about greatly so let's err on the side of simple and well- + // tested. + if (ssize_t excess_updates = all_updates_.size() - checkpoint_ptr->drop()) { + assert(excess_updates > 0); // should never be negative + + // need a copy because we'll be mutating all_updates_ in the loop + auto excess_view = + all_updates_ | std::views::reverse | std::views::take(excess_updates); + std::vector excess(excess_view.begin(), excess_view.end()); + for (const auto& [idx, old, _] : excess) { + all_updates_.emplace_back(idx, elements_[idx], old); + if (idx < size_) updates_.emplace_back(idx, elements_[idx], old); + elements_[idx] = old; + } + } + // Ok, let's get ourselves to the same place as the checkpoint // we want to minimize the size of the visible buffer, so let's shrink ourselves diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index 30920d53..e0b10673 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -108,14 +108,18 @@ class NumberNodeCheckpoint_ : public DiffCheckpoint { assert(this->drop() == 0); } - auto detach_slice_cache() { + auto detach_updates() { + auto updates = DiffCheckpoint::detach_updates(); + using join_type = decltype(std::move(slice_caches_) | std::views::join); - if (slice_caches_.empty()) return std::optional(); + if (slice_caches_.empty()) { + return std::make_tuple(std::move(updates), std::optional()); + } auto joined = std::move(slice_caches_) | std::views::join; assert(slice_caches_.empty()); - return std::optional(std::move(joined)); + return std::make_tuple(std::move(updates), std::optional(std::move(joined))); } void revert_updates(std::vector updates, slice_cache_type slice_cache) { @@ -1052,13 +1056,27 @@ void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type& checkpoi // todo: assert that this checkpoint is the latest - auto updates = checkpoint_ptr->detach_updates(); - auto slice_cache = checkpoint_ptr->detach_slice_cache(); // this is an std::optional<...>! + // Check if there are any changes not otherwise tracked by a checkpoint that we need + // to revert first. + // A better way would be to implement a partial revert on our state class, but this + // is not a path we care about greatly so let's err on the side of simple and well- + // tested. + if (ssize_t excess_updates = state_data->diff().size() - checkpoint_ptr->drop()) { + assert(excess_updates > 0); + for ( + const auto& [idx, old, _] : + state_data->diff() | std::views::reverse | std::views::take(excess_updates) + ) { + state_data->set(idx, old); + } + } - if (slice_cache.has_value()) { + auto [updates, optional_slice_cache] = checkpoint_ptr->detach_updates(); + + if (optional_slice_cache.has_value()) { assert(sum_constraints_.size() > 0); - auto slices_rit = std::ranges::rbegin(*slice_cache); + auto slices_rit = std::ranges::rbegin(*optional_slice_cache); for (const auto& [idx, old, _] : std::move(updates) | std::views::reverse) { state_data->set(idx, old); @@ -1072,6 +1090,8 @@ void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type& checkpoi state_data->set(idx, old); } } + + checkpoint_ptr->drop() = state_data->diff().size(); } void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const { diff --git a/tests/cpp/nodes/test_collections.cpp b/tests/cpp/nodes/test_collections.cpp index b7820e5f..7c2f01d1 100644 --- a/tests/cpp/nodes/test_collections.cpp +++ b/tests/cpp/nodes/test_collections.cpp @@ -794,6 +794,16 @@ TEST_CASE("SetNode") { // this is a smoke test because there is no public way to check // that the checkpoint didn't get copied over } + + THEN("We can commit, mutate, then revert") { + graph.propose(state); + + set_ptr->exchange(state, 1, 2); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint0)); + + // CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + } } WHEN("We mutate the state and then create a checkpoint before commiting") { @@ -853,6 +863,8 @@ TEST_CASE("SetNode") { } } + // TODO: within one propagation + WHEN("We do several mutations and create several checkpoints within the same commit") { auto checkpoint0 = set_ptr->checkpoint(state); @@ -873,8 +885,6 @@ TEST_CASE("SetNode") { graph.propose(state); THEN("we can go backwards through them without commiting and everything is correct") { - // TODO: check mutating before assigning - set_ptr->assign_from_checkpoint(state, std::move(checkpoint4)); CHECK_THAT(set_ptr->view(state), RangeEquals({2})); @@ -926,6 +936,16 @@ TEST_CASE("SetNode") { CHECK_THAT(set_ptr->view(state), RangeEquals({4, 1})); } + THEN("We can assign from a checkpoint, mutate, and then assign again") { + set_ptr->assign_from_checkpoint(state, std::move(checkpoint4)); + CHECK_THAT(set_ptr->view(state), RangeEquals({2})); + + set_ptr->assign(state, {3, 0, 4}); + + set_ptr->assign_from_checkpoint(state, std::move(checkpoint3)); + CHECK_THAT(set_ptr->view(state), RangeEquals({4, 2})); + } + WHEN("We do some fuzzing with checkpoints") { auto rng = std::default_random_engine(); diff --git a/tests/cpp/nodes/test_numbers.cpp b/tests/cpp/nodes/test_numbers.cpp index 0e10ae0d..5d28e3ea 100644 --- a/tests/cpp/nodes/test_numbers.cpp +++ b/tests/cpp/nodes/test_numbers.cpp @@ -2002,6 +2002,15 @@ TEST_CASE("IntegerNode") { CHECK_THAT(ptr->view(state), RangeEquals({-4, -4, -2, -2, 0, 0, 2, 2, 4, 4})); } + + THEN("We can commit, mutate, then assign from the checkpoint") { + graph.propose(state); + + ptr->set_value(state, 9, 0); // [-2, -4, -4, 1, 0, 0, 2, 2, 4, 0] + ptr->assign_from_checkpoint(state, checkpoint); + + CHECK_THAT(ptr->view(state), RangeEquals({-4, -4, -2, -2, 0, 0, 2, 2, 4, 4})); + } } AND_WHEN("We mutate, checkpoint the state, and then mutate again") { From e845381119902ce17826a24031abdc4246ed5ce2 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Mon, 27 Jul 2026 09:19:23 -0700 Subject: [PATCH 11/21] Add test for BinaryNode::assign_from_checkpoint --- dwave/optimization/src/nodes/collections.cpp | 23 ++++++++++++-------- tests/cpp/nodes/test_numbers.cpp | 12 ++++++++++ 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index a08f3a09..11e122fc 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -145,10 +145,10 @@ class CollectionStateData_ : public NodeStateData, public CheckpointableState { auto excess_view = all_updates_ | std::views::reverse | std::views::take(excess_updates); std::vector excess(excess_view.begin(), excess_view.end()); + + // now do the mutation for (const auto& [idx, old, _] : excess) { - all_updates_.emplace_back(idx, elements_[idx], old); - if (idx < size_) updates_.emplace_back(idx, elements_[idx], old); - elements_[idx] = old; + set_(idx, old); } } @@ -159,12 +159,7 @@ class CollectionStateData_ : public NodeStateData, public CheckpointableState { while (size_ > checkpoint_ptr->size()) shrink(); for (const auto& [idx, old, _] : checkpoint_ptr->detach_updates() | std::views::reverse) { - if (elements_[idx] == old) continue; // nothing to do - - all_updates_.emplace_back(idx, elements_[idx], old); - if (idx < size_) updates_.emplace_back(idx, elements_[idx], old); - - elements_[idx] = old; + set_(idx, old); } // now that we've filled in our buffer, grow until we're the correct size @@ -282,6 +277,16 @@ class CollectionStateData_ : public NodeStateData, public CheckpointableState { ssize_t size_diff() const { return size_ - previous_size_; } private: + void set_(ssize_t index, double value) { + assert(0 <= index and static_cast(index) < elements_.size()); + + if (elements_[index] == value) return; + + all_updates_.emplace_back(index, elements_[index], value); + if (index < size_) updates_.emplace_back(index, elements_[index], value); + elements_[index] = value; + } + friend CollectionCheckpoint_; // The elements in the collection diff --git a/tests/cpp/nodes/test_numbers.cpp b/tests/cpp/nodes/test_numbers.cpp index 5d28e3ea..45d228b0 100644 --- a/tests/cpp/nodes/test_numbers.cpp +++ b/tests/cpp/nodes/test_numbers.cpp @@ -295,6 +295,18 @@ TEST_CASE("BinaryNode") { CHECK(static_cast(ptr->diff(state).size()) == 2 * exchange_count_ground); } } + + AND_WHEN("We create a checkpoint to that state") { + auto checkpoint = ptr->checkpoint(state); // 0, 1, 0, 1, 0, 1, 0, 1, 0, 1 + + THEN("We can mutate and then assign from that checkpoint") { + ptr->set_value(state, 0, 1); // 0, 1, 0, 1, 0, 1, 0, 1, 0, 1 + graph.propose(state); + + ptr->assign_from_checkpoint(state, checkpoint); + CHECK_THAT(ptr->view(state), RangeEquals({0, 1, 0, 1, 0, 1, 0, 1, 0, 1})); + } + } } } From 4402568ac179067ae275345fa8b51b1ec11b5665 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Mon, 27 Jul 2026 11:45:24 -0700 Subject: [PATCH 12/21] Make all DecisionNodes implement checkpointing --- .../include/dwave-optimization/graph.hpp | 7 ++ .../dwave-optimization/nodes/collections.hpp | 24 ++++- .../dwave-optimization/nodes/numbers.hpp | 9 +- .../dwave-optimization/nodes/testing.hpp | 22 +++++ dwave/optimization/src/nodes/collections.cpp | 92 ++++++++++++++++++- ...eature-checkpointing-b770d2f2b66f648d.yaml | 2 +- tests/cpp/nodes/test_collections.cpp | 87 ++++++++++++++++++ 7 files changed, 231 insertions(+), 12 deletions(-) diff --git a/dwave/optimization/include/dwave-optimization/graph.hpp b/dwave/optimization/include/dwave-optimization/graph.hpp index b05baa0e..ffd32fb1 100644 --- a/dwave/optimization/include/dwave-optimization/graph.hpp +++ b/dwave/optimization/include/dwave-optimization/graph.hpp @@ -436,6 +436,13 @@ class DecisionNode : public Decision, public virtual Node { /// Decision nodes by definition do not have a deterministic state. bool deterministic_state() const final { return false; } + /// Set the current state to match the one at the time the given checkpoint was created. + virtual void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const = 0; + virtual void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const = 0; + + /// Get a checkpoint, an IOU that can be used to return the node to its current state. + virtual checkpoint_type checkpoint(State& state) const = 0; + /// Decisions don't have predecessors so no one should be calling update(). /// Always throws a logic_error. [[noreturn]] void update(State& state, int index) const override; diff --git a/dwave/optimization/include/dwave-optimization/nodes/collections.hpp b/dwave/optimization/include/dwave-optimization/nodes/collections.hpp index 68aeafae..c1c17122 100644 --- a/dwave/optimization/include/dwave-optimization/nodes/collections.hpp +++ b/dwave/optimization/include/dwave-optimization/nodes/collections.hpp @@ -32,14 +32,14 @@ class CollectionNode : public ArrayOutputMixin, public DecisionNode { // Set the node's state, tracking the diff. void assign(State& state, std::vector values) const; - /// Set the current state to match the one at the time the given checkpoint was created. - void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const; - void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const; + /// @copydoc DecisionNode::assign_from_checkpoint() + void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const override; + void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const override; const double* buff(const State& state) const override; - /// Get a checkpoint, an IOU that can be used to return the node to its current state. - checkpoint_type checkpoint(State& state) const; + /// @copydoc DecisionNode::checkpoint() + checkpoint_type checkpoint(State& state) const override; void commit(State&) const override; @@ -107,6 +107,13 @@ class DisjointBitSetsNode : public DecisionNode { // i.e. the set `range(primary_set_size)`. DisjointBitSetsNode(ssize_t primary_set_size, ssize_t num_disjoint_sets); + /// @copydoc DecisionNode::assign_from_checkpoint() + void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const override; + void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const override; + + /// @copydoc DecisionNode::checkpoint() + checkpoint_type checkpoint(State& state) const override; + void commit(State&) const override; ssize_t get_containing_set_index(State& state, ssize_t element_i) const; @@ -179,6 +186,13 @@ class DisjointListsNode : public DecisionNode { // i.e. the set `range(primary_set_size)`. DisjointListsNode(ssize_t primary_set_size, ssize_t num_disjoint_lists); + /// @copydoc DecisionNode::assign_from_checkpoint() + void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const override; + void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const override; + + /// @copydoc DecisionNode::checkpoint() + checkpoint_type checkpoint(State& state) const override; + void commit(State&) const override; ssize_t get_disjoint_list_size(State& state, ssize_t list_index) const; diff --git a/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp b/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp index 34cc9069..44a08bc6 100644 --- a/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp +++ b/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp @@ -121,7 +121,8 @@ class NumberNode : public ArrayOutputMixin, public DecisionNode { // NumberNode methods ***************************************************** - std::unique_ptr checkpoint(State& state) const; + /// @copydoc DecisionNode::checkpoint() + checkpoint_type checkpoint(State& state) const override; // In the given state, swap the value of index i with the value of index j. // Users may pass the slices (per sum constraint) that each index lies on. @@ -291,9 +292,9 @@ class IntegerNode : public NumberNode { // IntegerNode methods **************************************************** - /// Set the current state to match the one at the time the given checkpoint was created. - void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const; - void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const; + /// @copydoc DecisionNode::assign_from_checkpoint() + void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const override; + void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const override; // Set the value at the given index in the given state. // Users may pass the slices (per sum constraint) that each index lies on. diff --git a/dwave/optimization/include/dwave-optimization/nodes/testing.hpp b/dwave/optimization/include/dwave-optimization/nodes/testing.hpp index 53e08a61..16179ead 100644 --- a/dwave/optimization/include/dwave-optimization/nodes/testing.hpp +++ b/dwave/optimization/include/dwave-optimization/nodes/testing.hpp @@ -100,6 +100,28 @@ class DynamicArrayTestingNode : public ArrayOutputMixin, public Decis void revert(State&) const override; void update(State&, int) const override; + // Overloads required by the DecisionNode ABC ***************************** + + // DynamicArrayTestingNode does not impement checkpointing + [[noreturn]] void assign_from_checkpoint( + State& state, + checkpoint_type& checkpoint + ) const override { + assert(false and "not implemented"); + unreachable(); + } + [[noreturn]] void assign_from_checkpoint( + State& state, + checkpoint_type&& checkpoint + ) const override { + assert(false and "not implemented"); + unreachable(); + } + [[noreturn]] virtual checkpoint_type checkpoint(State& state) const override { + assert(false and "not implemented"); + unreachable(); + } + // State mutation methods ************************************************* // Grow the array by a single row of the given values. diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index 11e122fc..1bb52235 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -440,7 +440,17 @@ ssize_t CollectionNode::size_diff(const State& state) const { return data_ptr_(state)->size_diff(); } -struct DisjointBitSetsNodeData_ : NodeStateData { +// DisjointBitSetsNode is on the way out, so let's do the simplest possible +// implementation for now. +class DisjointBitSetsCheckpoint_ : public LinkedListCheckpoint { + public: + DisjointBitSetsCheckpoint_(CheckpointableState& state, const std::ranges::range auto& buff) : + LinkedListCheckpoint(state), buffer(buff.begin(), buff.end()) {} + + std::vector buffer; +}; + +struct DisjointBitSetsNodeData_ : CheckpointableState, NodeStateData { DisjointBitSetsNodeData_(ssize_t primary_set_size, ssize_t num_disjoint_sets) : primary_set_size(primary_set_size), num_disjoint_sets(num_disjoint_sets) { data.resize(primary_set_size * num_disjoint_sets, 0); @@ -489,6 +499,21 @@ struct DisjointBitSetsNodeData_ : NodeStateData { } } + void assign(std::span buff) { + assert(data.size() == buff.size()); + + for (ssize_t disjoint_set = 0; disjoint_set < num_disjoint_sets; ++disjoint_set) { + const ssize_t start = disjoint_set * primary_set_size; + const ssize_t stop = start + primary_set_size; + for (ssize_t i = start; i < stop; ++i) { + if (data[i] != buff[i]) { + diffs[disjoint_set].emplace_back(i % primary_set_size, data[i], buff[i]); + data[i] = buff[i]; + } + } + } + } + void swap_between_sets(ssize_t from_disjoint_set, ssize_t to_disjoint_set, ssize_t element) { double& el0 = data[from_disjoint_set * primary_set_size + element]; double& el1 = data[to_disjoint_set * primary_set_size + element]; @@ -556,6 +581,28 @@ void DisjointBitSetsNode::initialize_state( ); } +void DisjointBitSetsNode::assign_from_checkpoint( + State& state, + std::unique_ptr& checkpoint +) const { + const DisjointBitSetsCheckpoint_* checkpoint_ptr = + static_cast(checkpoint.get()); + data_ptr_(state)->assign(checkpoint_ptr->buffer); +} + +void DisjointBitSetsNode::assign_from_checkpoint( + State& state, + std::unique_ptr&& checkpoint +) const { + assign_from_checkpoint(state, checkpoint); // use the lvalue version + checkpoint.reset(); +} + +std::unique_ptr DisjointBitSetsNode::checkpoint(State& state) const { + auto* state_ptr = data_ptr_(state); + return std::make_unique(*state_ptr, state_ptr->data); +} + void DisjointBitSetsNode::commit(State& state) const { data_ptr_(state)->commit(); } @@ -610,7 +657,20 @@ double DisjointBitSetNode::min() const { return 0; } double DisjointBitSetNode::max() const { return 1; } -struct DisjointListStateData_ : NodeStateData { +// DisjointListsNode is on the way out, so let's do the simplest possible +// implementation for now. +class DisjointListsCheckpoint_ : public LinkedListCheckpoint { + public: + DisjointListsCheckpoint_( + CheckpointableState& state, + const std::vector>& lists + ) : + LinkedListCheckpoint(state), lists(lists) {} + + std::vector> lists; +}; + +struct DisjointListStateData_ : CheckpointableState, NodeStateData { DisjointListStateData_(ssize_t primary_set_size, ssize_t num_disjoint_lists) : primary_set_size(primary_set_size) { lists.resize(num_disjoint_lists); @@ -840,6 +900,34 @@ DisjointListsNode::DisjointListsNode(ssize_t primary_set_size, ssize_t num_disjo if (num_disjoint_lists < 1) throw std::invalid_argument("num_disjoint_lists must be positive"); } +void DisjointListsNode::assign_from_checkpoint( + State& state, + std::unique_ptr& checkpoint +) const { + auto* state_ptr = data_ptr_(state); + + const DisjointListsCheckpoint_* checkpoint_ptr = + static_cast(checkpoint.get()); + + ssize_t list_index = 0; + for (const std::vector& list : checkpoint_ptr->lists) { + state_ptr->set_state(list_index++, list); + } +} + +void DisjointListsNode::assign_from_checkpoint( + State& state, + std::unique_ptr&& checkpoint +) const { + assign_from_checkpoint(state, checkpoint); // use the lvalue version + checkpoint.reset(); +} + +std::unique_ptr DisjointListsNode::checkpoint(State& state) const { + auto* state_ptr = data_ptr_(state); + return std::make_unique(*state_ptr, state_ptr->lists); +} + void DisjointListsNode::initialize_state(State& state) const { emplace_data_ptr_( state, this->primary_set_size(), this->num_disjoint_lists() diff --git a/releasenotes/notes/feature-checkpointing-b770d2f2b66f648d.yaml b/releasenotes/notes/feature-checkpointing-b770d2f2b66f648d.yaml index c82063e4..014fda90 100644 --- a/releasenotes/notes/feature-checkpointing-b770d2f2b66f648d.yaml +++ b/releasenotes/notes/feature-checkpointing-b770d2f2b66f648d.yaml @@ -3,5 +3,5 @@ features: - | Add a C++ ``Graph::propose(State&)`` overload that propagates and commits. - | - Add checkpointing to ``CollectionNode``. + Add checkpointing to all decision nodes. See `#510 `_. diff --git a/tests/cpp/nodes/test_collections.cpp b/tests/cpp/nodes/test_collections.cpp index 7c2f01d1..adceabdb 100644 --- a/tests/cpp/nodes/test_collections.cpp +++ b/tests/cpp/nodes/test_collections.cpp @@ -162,6 +162,47 @@ TEST_CASE("DisjointBitSetsNode") { CHECK(std::ranges::equal(sets[1]->view(state), std::vector{1, 0, 1, 0, 0})); CHECK(std::ranges::equal(sets[2]->view(state), std::vector{0, 1, 0, 1, 0})); } + + AND_WHEN("We create a checkpoint to that state") { + auto checkpoint = ptr->checkpoint(state); + + THEN("We can mutate and then assign from that checkpoint") { + ptr->swap_between_sets(state, 0, 1, 0); + graph.propose(state); + + ptr->assign_from_checkpoint(state, std::move(checkpoint)); + assert(not checkpoint); // was reset + + CHECK_THAT(sets[0]->view(state), RangeEquals({0, 0, 0, 0, 1})); + CHECK_THAT(sets[1]->view(state), RangeEquals({1, 0, 1, 0, 0})); + } + + THEN("We can assign, mutate, and then reuse the checkpoint") { + ptr->swap_between_sets(state, 0, 1, 0); + CHECK_THAT(sets[0]->view(state), RangeEquals({1, 0, 0, 0, 1})); + CHECK_THAT(sets[1]->view(state), RangeEquals({0, 0, 1, 0, 0})); + CHECK_THAT(sets[2]->view(state), RangeEquals({0, 1, 0, 1, 0})); + + graph.propose(state); + + ptr->assign_from_checkpoint(state, checkpoint); + CHECK_THAT(sets[0]->view(state), RangeEquals({0, 0, 0, 0, 1})); + CHECK_THAT(sets[1]->view(state), RangeEquals({1, 0, 1, 0, 0})); + CHECK_THAT(sets[2]->view(state), RangeEquals({0, 1, 0, 1, 0})); + + ptr->swap_between_sets(state, 1, 2, 1); + CHECK_THAT(sets[0]->view(state), RangeEquals({0, 0, 0, 0, 1})); + CHECK_THAT(sets[1]->view(state), RangeEquals({1, 1, 1, 0, 0})); + CHECK_THAT(sets[2]->view(state), RangeEquals({0, 0, 0, 1, 0})); + + graph.propose(state); + + ptr->assign_from_checkpoint(state, std::move(checkpoint)); + CHECK_THAT(sets[0]->view(state), RangeEquals({0, 0, 0, 0, 1})); + CHECK_THAT(sets[1]->view(state), RangeEquals({1, 0, 1, 0, 0})); + CHECK_THAT(sets[2]->view(state), RangeEquals({0, 1, 0, 1, 0})); + } + } } AND_WHEN("We initialize an empty state") { @@ -322,6 +363,52 @@ TEST_CASE("DisjointListsNode") { CHECK(std::ranges::equal(lists[1]->view(state), std::vector{2, 0})); CHECK(std::ranges::equal(lists[2]->view(state), std::vector{1, 3})); } + + AND_WHEN("We create a checkpoint to that state") { + auto checkpoint = ptr->checkpoint(state); + + THEN("We can mutate and then assign from that checkpoint") { + ptr->pop_to_list(state, 1, 0, 0, 1); + CHECK_THAT(lists[0]->view(state), RangeEquals({4, 2})); + CHECK_THAT(lists[1]->view(state), RangeEquals({0})); + CHECK_THAT(lists[2]->view(state), RangeEquals({1, 3})); + + graph.propose(state); + + ptr->assign_from_checkpoint(state, std::move(checkpoint)); + assert(not checkpoint); // was reset + + CHECK_THAT(lists[0]->view(state), RangeEquals({4})); + CHECK_THAT(lists[1]->view(state), RangeEquals({2, 0})); + CHECK_THAT(lists[2]->view(state), RangeEquals({1, 3})); + } + + THEN("We can assign, mutate, and then reuse the checkpoint") { + ptr->pop_to_list(state, 1, 0, 0, 1); + CHECK_THAT(lists[0]->view(state), RangeEquals({4, 2})); + CHECK_THAT(lists[1]->view(state), RangeEquals({0})); + CHECK_THAT(lists[2]->view(state), RangeEquals({1, 3})); + + graph.propose(state); + + ptr->assign_from_checkpoint(state, checkpoint); + CHECK_THAT(lists[0]->view(state), RangeEquals({4})); + CHECK_THAT(lists[1]->view(state), RangeEquals({2, 0})); + CHECK_THAT(lists[2]->view(state), RangeEquals({1, 3})); + + ptr->pop_to_list(state, 1, 1, 2, 2); + CHECK_THAT(lists[0]->view(state), RangeEquals({4})); + CHECK_THAT(lists[1]->view(state), RangeEquals({2})); + CHECK_THAT(lists[2]->view(state), RangeEquals({1, 3, 0})); + + graph.propose(state); + + ptr->assign_from_checkpoint(state, std::move(checkpoint)); + CHECK_THAT(lists[0]->view(state), RangeEquals({4})); + CHECK_THAT(lists[1]->view(state), RangeEquals({2, 0})); + CHECK_THAT(lists[2]->view(state), RangeEquals({1, 3})); + } + } } THEN("We get an error when trying to initialize invalid partitions") { From ec4abfc8999f02c9c9bdea3bd846087b6c822a6e Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Tue, 4 Aug 2026 10:51:47 -0700 Subject: [PATCH 13/21] Expand docstrings for checkpoint implementations --- dwave/optimization/src/nodes/_checkpoints.hpp | 72 +++++++++++++------ 1 file changed, 50 insertions(+), 22 deletions(-) diff --git a/dwave/optimization/src/nodes/_checkpoints.hpp b/dwave/optimization/src/nodes/_checkpoints.hpp index 8e1866d7..f6187ca0 100644 --- a/dwave/optimization/src/nodes/_checkpoints.hpp +++ b/dwave/optimization/src/nodes/_checkpoints.hpp @@ -23,13 +23,42 @@ namespace dwave::optimization { -class LinkedListCheckpoint; +class CheckpointableState; +// A LinkedListCheckpoint is one checkpoint in a chain of checkpoints implemented +// as a doubly-linked list. +class LinkedListCheckpoint : public NodeStateCheckpoint { + public: + LinkedListCheckpoint() = delete; + // We're not moveable or copy-able because NodeStateCheckpoint is not. + + LinkedListCheckpoint(CheckpointableState& state); + + ~LinkedListCheckpoint() override; + + protected: + friend CheckpointableState; + + // The next-oldest checkpoint in the chain. Can be nullptr which indicates + // that this is the oldest checkpoint. + LinkedListCheckpoint* prev_ptr_; + + // The next-newest checkpoint in the chain or, if this is the newest + // newest checkpoint, will point to the node state. + // Is usually not nullptr unless the state has been destructed before the + // checkpoint has. + std::variant next_ptr_; +}; + +// A mixin class for states to work with LinkedListCheckpoints. class CheckpointableState { public: CheckpointableState() = default; - CheckpointableState(const CheckpointableState&) {} // the checkpoint pointer is not copied + // When CheckpointableState is copied, we don't want the new state to inherit + // its checkpoints. + CheckpointableState(const CheckpointableState&) {} + CheckpointableState(CheckpointableState&&) = default; CheckpointableState& operator=(const CheckpointableState&) = delete; @@ -43,55 +72,54 @@ class CheckpointableState { return static_cast(prev_ptr_); } - private: // todo: private? + private: friend LinkedListCheckpoint; // The name is a bit confusing, but by making it match LinkedListCheckpoint::prev_ptr_ // it makes the implementations of the various visit methods clearer. - LinkedListCheckpoint* prev_ptr_ = nullptr; // Will be nullptr if there are no checkpoints -}; - -class LinkedListCheckpoint : public NodeStateCheckpoint { - public: - LinkedListCheckpoint() = delete; - // We're not moveable or copyable because NodeStateCheckpoint is not. - - LinkedListCheckpoint(CheckpointableState& state); - - ~LinkedListCheckpoint() override; - - protected: // todo: private? - friend CheckpointableState; - - LinkedListCheckpoint* prev_ptr_; - - // Is usually not nullptr unless the state has been destructed - std::variant next_ptr_; + // Will be nullptr if there are no checkpoints + LinkedListCheckpoint* prev_ptr_ = nullptr; }; +// A DiffCheckpoint is a type of linked list checkpoint that stores the diffs +// since it was created. class DiffCheckpoint : public LinkedListCheckpoint { public: DiffCheckpoint(CheckpointableState& state, std::span diff); ~DiffCheckpoint() override; + // Add updates associated with a commit to the checkpoint. The checkpoint + // therefore stores the information it needs to later undo those changes. void commit_updates(std::vector updates); + // Clear all the updates held by the checkpoint and return them to the + // caller. auto detach_updates() { auto updates = std::move(updates_) | std::views::join; assert(updates_.empty()); return updates; } + // 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. ssize_t& drop() { return drop_; } + // Add updates associated with a revert to the checkpoint. The checkpoint + // therefore stores the information it needs to later undo those changes. void revert_updates(std::vector updates); protected: DiffCheckpoint(CheckpointableState& state, ssize_t drop); private: + // We store the updates as a vector-of-vectors in order to make them fast + // to append. std::vector> updates_; + + // See drop() docstring. ssize_t drop_; }; From f54877ddef376ea6626150b9b6900efa26296a08 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Thu, 6 Aug 2026 11:32:15 -0700 Subject: [PATCH 14/21] Address nonfunctional comments from code review --- .../include/dwave-optimization/nodes/testing.hpp | 2 +- dwave/optimization/src/nodes/_checkpoints.cpp | 16 +++++++--------- dwave/optimization/src/nodes/_state.hpp | 2 +- dwave/optimization/src/nodes/collections.cpp | 3 +-- dwave/optimization/src/nodes/numbers.cpp | 4 ++++ tests/cpp/nodes/test_collections.cpp | 4 +--- tests/cpp/nodes/test_numbers.cpp | 4 ++-- 7 files changed, 17 insertions(+), 18 deletions(-) diff --git a/dwave/optimization/include/dwave-optimization/nodes/testing.hpp b/dwave/optimization/include/dwave-optimization/nodes/testing.hpp index 16179ead..01f88406 100644 --- a/dwave/optimization/include/dwave-optimization/nodes/testing.hpp +++ b/dwave/optimization/include/dwave-optimization/nodes/testing.hpp @@ -102,7 +102,7 @@ class DynamicArrayTestingNode : public ArrayOutputMixin, public Decis // Overloads required by the DecisionNode ABC ***************************** - // DynamicArrayTestingNode does not impement checkpointing + // DynamicArrayTestingNode does not implement checkpointing [[noreturn]] void assign_from_checkpoint( State& state, checkpoint_type& checkpoint diff --git a/dwave/optimization/src/nodes/_checkpoints.cpp b/dwave/optimization/src/nodes/_checkpoints.cpp index 4b3fe59b..c45bbfe8 100644 --- a/dwave/optimization/src/nodes/_checkpoints.cpp +++ b/dwave/optimization/src/nodes/_checkpoints.cpp @@ -53,15 +53,13 @@ DiffCheckpoint::DiffCheckpoint(CheckpointableState& state, std::span(prev_ptr_); - assert(prev_ptr->drop_ == 0); - for (auto& updates : updates_) prev_ptr->commit_updates(std::move(updates)); - prev_ptr->drop_ = drop_; + // If we're not the oldest checkpoint, we need to transfer our information + // over so it's not lost + if (auto* prev_ptr = static_cast(prev_ptr_)) { + assert(prev_ptr->drop_ == 0); + for (auto& updates : updates_) prev_ptr->commit_updates(std::move(updates)); + prev_ptr->drop_ = drop_; + } } void DiffCheckpoint::commit_updates(std::vector updates) { diff --git a/dwave/optimization/src/nodes/_state.hpp b/dwave/optimization/src/nodes/_state.hpp index 5a2da6cb..43986b78 100644 --- a/dwave/optimization/src/nodes/_state.hpp +++ b/dwave/optimization/src/nodes/_state.hpp @@ -165,7 +165,7 @@ class ArrayStateData { size_ = buffer.size(); } - // Commit the changes and clear the diff by returning the diff buffer. + // Revert the changes and clear the diff by returning the diff buffer. std::vector revert_and_detach() { assert(previous_size_ >= 0); buffer.resize(previous_size_); diff --git a/dwave/optimization/src/nodes/collections.cpp b/dwave/optimization/src/nodes/collections.cpp index 1bb52235..c117118c 100644 --- a/dwave/optimization/src/nodes/collections.cpp +++ b/dwave/optimization/src/nodes/collections.cpp @@ -585,8 +585,7 @@ void DisjointBitSetsNode::assign_from_checkpoint( State& state, std::unique_ptr& checkpoint ) const { - const DisjointBitSetsCheckpoint_* checkpoint_ptr = - static_cast(checkpoint.get()); + const auto* checkpoint_ptr = static_cast(checkpoint.get()); data_ptr_(state)->assign(checkpoint_ptr->buffer); } diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index e0b10673..bd6ad8bd 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -238,6 +238,10 @@ void NumberNodeStateData::revert() { slice_cache_.clear(); // Empty the slice cache. ArrayNodeStateData::revert(); // Revert changes to the buffer. } + + // everything should have been cleared out regardless of which path we took + assert(this->diff().empty()); + assert(slice_cache_.empty()); } void NumberNodeStateData::update( diff --git a/tests/cpp/nodes/test_collections.cpp b/tests/cpp/nodes/test_collections.cpp index adceabdb..597ab0eb 100644 --- a/tests/cpp/nodes/test_collections.cpp +++ b/tests/cpp/nodes/test_collections.cpp @@ -889,7 +889,7 @@ TEST_CASE("SetNode") { set_ptr->assign_from_checkpoint(state, std::move(checkpoint0)); - // CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); + CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1})); } } @@ -950,8 +950,6 @@ TEST_CASE("SetNode") { } } - // TODO: within one propagation - WHEN("We do several mutations and create several checkpoints within the same commit") { auto checkpoint0 = set_ptr->checkpoint(state); diff --git a/tests/cpp/nodes/test_numbers.cpp b/tests/cpp/nodes/test_numbers.cpp index 45d228b0..7c55d558 100644 --- a/tests/cpp/nodes/test_numbers.cpp +++ b/tests/cpp/nodes/test_numbers.cpp @@ -299,8 +299,8 @@ TEST_CASE("BinaryNode") { AND_WHEN("We create a checkpoint to that state") { auto checkpoint = ptr->checkpoint(state); // 0, 1, 0, 1, 0, 1, 0, 1, 0, 1 - THEN("We can mutate and then assign from that checkpoint") { - ptr->set_value(state, 0, 1); // 0, 1, 0, 1, 0, 1, 0, 1, 0, 1 + THEN("We can mutate, then propose, and then assign from that checkpoint") { + ptr->set_value(state, 0, 1); // 1, 1, 0, 1, 0, 1, 0, 1, 0, 1 graph.propose(state); ptr->assign_from_checkpoint(state, checkpoint); From 7343b417207d4cc04879e5401fd3bee4efc3ae0b Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Thu, 6 Aug 2026 14:57:59 -0700 Subject: [PATCH 15/21] Address functional comments from code review --- dwave/optimization/src/nodes/_checkpoints.hpp | 4 ++++ dwave/optimization/src/nodes/numbers.cpp | 11 +++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/dwave/optimization/src/nodes/_checkpoints.hpp b/dwave/optimization/src/nodes/_checkpoints.hpp index f6187ca0..af39e978 100644 --- a/dwave/optimization/src/nodes/_checkpoints.hpp +++ b/dwave/optimization/src/nodes/_checkpoints.hpp @@ -71,6 +71,10 @@ class CheckpointableState { T* checkpoint_ptr() { return static_cast(prev_ptr_); } + template T> + const T* checkpoint_ptr() const { + return static_cast(prev_ptr_); + } private: friend LinkedListCheckpoint; diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index bd6ad8bd..c547d51f 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -180,6 +180,10 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat assert(slice_cache_.empty()); } + const NumberNodeCheckpoint_* last_checkpoint() const { + return checkpoint_ptr(); + } + /// Revert the state dependent data of NumberNode. void revert(); @@ -1058,7 +1062,7 @@ void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type& checkpoi auto* checkpoint_ptr = static_cast(checkpoint.get()); - // todo: assert that this checkpoint is the latest + assert(checkpoint_ptr == state_data->last_checkpoint()); // Check if there are any changes not otherwise tracked by a checkpoint that we need // to revert first. @@ -1067,11 +1071,14 @@ void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type& checkpoi // tested. if (ssize_t excess_updates = state_data->diff().size() - checkpoint_ptr->drop()) { assert(excess_updates > 0); + for ( const auto& [idx, old, _] : state_data->diff() | std::views::reverse | std::views::take(excess_updates) ) { - state_data->set(idx, old); + // This is a *very* expensive call. But, again, we're not too worried about + // performance here. + this->set_value(state, idx, old); } } From 885c8f1b774b164df65dc63d191f42b78e76108f Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Fri, 7 Aug 2026 14:29:56 -0700 Subject: [PATCH 16/21] Update NumberNodeStateData to hold a reference to NumberNode --- dwave/optimization/src/nodes/numbers.cpp | 99 +++++++++++++----------- 1 file changed, 53 insertions(+), 46 deletions(-) diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index a5c7e1e7..42731c61 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -148,13 +148,19 @@ class NumberNodeCheckpoint_ : public DiffCheckpoint { class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableState { public: // User does not provide sum constraints. - NumberNodeStateData(std::vector input) : ArrayNodeStateData(std::move(input)) {} + NumberNodeStateData(const NumberNode& node, std::vector input) : + ArrayNodeStateData(std::move(input)), sum_constraints_lhs(), slice_cache_(), node_(node) {} + // User provides sum constraints. NumberNodeStateData( + const NumberNode& node, std::vector input, std::vector> sum_constraints_lhs ) : - ArrayNodeStateData(std::move(input)), sum_constraints_lhs(std::move(sum_constraints_lhs)) {} + ArrayNodeStateData(std::move(input)), + sum_constraints_lhs(std::move(sum_constraints_lhs)), + slice_cache_(), + node_(node) {} std::unique_ptr checkpoint() { return std::make_unique(*this, this->diff(), this->slice_cache_); @@ -189,10 +195,10 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat /// Update the relevant sum constraints running sums (`lhs`) given that the /// value stored at `index` is changed by `difference`. - void update(const NumberNode& node, const ssize_t index, const double difference); + void update(const ssize_t index, const double difference); + /// Users may pass the slices (per sum constraint) that `index` lies on. void update( - const NumberNode& node, const ssize_t index, const double difference, std::vector slices @@ -213,6 +219,10 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat /// slice_cache_[i][j] = The slice of the `j`th sum constraint that the index /// of the `i`th update lies on. std::vector> slice_cache_; + + /// Hold a reference to the parent node. This class can outlive the node but + /// cannot be accessed except through it. + const NumberNode& node_; }; void NumberNodeStateData::revert() { @@ -249,11 +259,10 @@ void NumberNodeStateData::revert() { } void NumberNodeStateData::update( - const NumberNode& node, const ssize_t index, const double difference ) { - const auto& sum_constraints = node.sum_constraints(); + const auto& sum_constraints = node_.sum_constraints(); assert(sum_constraints.size() != 0); // Should only call where applicable. assert(difference != 0); // Should not call when no change occurs. assert(sum_constraints.size() == sum_constraints_lhs.size()); @@ -262,7 +271,7 @@ void NumberNodeStateData::update( cache_entry.reserve(sum_constraints.size()); // Get multidimensional indices for `index` so we can identify the slices // `index` lies on per sum constraint. - const std::vector multi_index = unravel_index(index, node.shape()); + const std::vector multi_index = unravel_index(index, node_.shape()); assert(sum_constraints.size() <= multi_index.size()); // For each sum constraint. for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { @@ -280,12 +289,11 @@ void NumberNodeStateData::update( } void NumberNodeStateData::update( - const NumberNode& node, const ssize_t index, const double difference, std::vector slices ) { - const auto& sum_constraints = node.sum_constraints(); + const auto& sum_constraints = node_.sum_constraints(); assert(sum_constraints.size() != 0); // Should only call where applicable. assert(difference != 0); // Should not call when no change occurs. assert(sum_constraints.size() == sum_constraints_lhs.size()); @@ -299,7 +307,7 @@ void NumberNodeStateData::update( /// If `axis == std::nullopt`, the array is treated as a flat array with a /// single slice. Otherwise, the slice is defined by unravel_index(). if (!axis.has_value()) return slices[i] == 0; - return slices[i] == unravel_index(index, node.shape())[*axis]; + return slices[i] == unravel_index(index, node_.shape())[*axis]; })()); sum_constraints_lhs[i][slices[i]] += difference; // Offset slice sum. } @@ -420,7 +428,7 @@ void NumberNode::initialize_state(State& state, std::vector&& number_dat } if (sum_constraints_.size() == 0) { // No sum constraints to consider. - emplace_data_ptr_(state, std::move(number_data)); + emplace_data_ptr_(state, *this, std::move(number_data)); } else { // Given the assignment to NumberNode `number_data`, compute the sum // of the values within each slice per sum constraint. @@ -431,7 +439,7 @@ void NumberNode::initialize_state(State& state, std::vector&& number_dat } emplace_data_ptr_( - state, std::move(number_data), std::move(sum_constraints_lhs) + state, *this, std::move(number_data), std::move(sum_constraints_lhs) ); } } @@ -664,15 +672,15 @@ void NumberNode::exchange( if (i_slices.has_value()) { assert(j_slices.has_value()); // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - state_data->update(*this, i, difference, *i_slices); + state_data->update(i, difference, *i_slices); // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - state_data->update(*this, j, -difference, *j_slices); + state_data->update(j, -difference, *j_slices); } else { assert(!j_slices.has_value()); // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - state_data->update(*this, i, difference); + state_data->update(i, difference); // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - state_data->update(*this, j, -difference); + state_data->update(j, -difference); } } } @@ -732,9 +740,9 @@ void NumberNode::clip_and_set_value( // If change occurred and sum constraint exist, update running sums. if (sum_constraints_.size() > 0) { if (slices.has_value()) { - state_data->update(*this, index, value - diff(state).back().old, *slices); + state_data->update(index, value - diff(state).back().old, *slices); } else { - state_data->update(*this, index, value - diff(state).back().old); + state_data->update(index, value - diff(state).back().old); } } } @@ -1091,7 +1099,7 @@ void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type& checkpoi for (const auto& [idx, old, _] : std::move(updates) | std::views::reverse) { state_data->set(idx, old); - state_data->update(*this, idx, old - diff(state).back().old, *(slices_rit++)); + state_data->update(idx, old - diff(state).back().old, *(slices_rit++)); } } else { assert(updates.empty() or sum_constraints_.empty()); @@ -1134,9 +1142,9 @@ void IntegerNode::set_value( // If change occurred and sum constraint exist, update running sums. if (sum_constraints_.size() > 0) { if (slices.has_value()) { - state_data->update(*this, index, value - diff(state).back().old, *slices); + state_data->update(index, value - diff(state).back().old, *slices); } else { - state_data->update(*this, index, value - diff(state).back().old); + state_data->update(index, value - diff(state).back().old); } } } @@ -1417,14 +1425,16 @@ struct BinaryNodeStateData : public NumberNodeStateData { }; // User does not provide sum constraints. - BinaryNodeStateData(std::vector input) : NumberNodeStateData(std::move(input)) {} + BinaryNodeStateData(const BinaryNode& node, std::vector input) : + NumberNodeStateData(node, std::move(input)) {} + // User provides sum constraints. BinaryNodeStateData( + const BinaryNode& node, std::vector input, - std::vector> sum_constraints_lhs, - const BinaryNode& node + std::vector> sum_constraints_lhs ) : - NumberNodeStateData(std::move(input), std::move(sum_constraints_lhs)) { + NumberNodeStateData(node, std::move(input), std::move(sum_constraints_lhs)) { compute_slice_indices_(node); } @@ -1437,10 +1447,10 @@ struct BinaryNodeStateData : public NumberNodeStateData { /// Update `sum_constraints_lhs` and `slice_indices` given that the value /// stored at `index` is changed by `difference`. - void update(const BinaryNode& node, const ssize_t index, const double difference); + void update(const ssize_t index, const double difference); + /// Users may pass the slices (per sum constraint) that `index` lies on. void update( - const BinaryNode& node, const ssize_t index, const double difference, std::vector slices @@ -1487,11 +1497,10 @@ void BinaryNodeStateData::revert() { } void BinaryNodeStateData::update( - const BinaryNode& node, const ssize_t index, const double difference ) { - const auto& sum_constraints = node.sum_constraints(); + const auto& sum_constraints = node_.sum_constraints(); assert(sum_constraints.size() != 0); // Should only call where applicable. assert(difference == 1 || difference == -1); assert(sum_constraints.size() == sum_constraints_lhs.size()); @@ -1500,7 +1509,7 @@ void BinaryNodeStateData::update( cache_entry.reserve(sum_constraints.size()); // Get multidimensional indices for `index` so we can identify the slices // `index` lies on per sum constraint. - const std::vector multi_index = unravel_index(index, node.shape()); + const std::vector multi_index = unravel_index(index, node_.shape()); assert(sum_constraints.size() <= multi_index.size()); // For each sum constraint. for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { @@ -1524,12 +1533,11 @@ void BinaryNodeStateData::update( } void BinaryNodeStateData::update( - const BinaryNode& node, const ssize_t index, const double difference, std::vector slices ) { - const auto& sum_constraints = node.sum_constraints(); + const auto& sum_constraints = node_.sum_constraints(); assert(sum_constraints.size() != 0); // Should only call where applicable. assert(difference == 1 || difference == -1); assert(sum_constraints.size() == sum_constraints_lhs.size()); @@ -1544,7 +1552,7 @@ void BinaryNodeStateData::update( /// If `axis == std::nullopt`, the array is treated as a flat array with a /// single slice. Otherwise, the slice is defined by multi_index. if (!axis.has_value()) return slices[i] == 0; - return slices[i] == unravel_index(index, node.shape())[*axis]; + return slices[i] == unravel_index(index, node_.shape())[*axis]; })()); sum_constraints_lhs[i][slices[i]] += difference; // Offset slice sum. // Update tracked indices. @@ -1608,7 +1616,7 @@ void BinaryNode::initialize_state(State& state, std::vector&& number_dat } if (sum_constraints_.size() == 0) { // No sum constraints to consider. - emplace_data_ptr_(state, std::move(number_data)); + emplace_data_ptr_(state, *this, std::move(number_data)); } else { // Given the assignment to NumberNode `number_data`, compute the sum of // the values within each slice per sum constraint. @@ -1619,8 +1627,7 @@ void BinaryNode::initialize_state(State& state, std::vector&& number_dat } emplace_data_ptr_( - state, std::move(number_data), std::move(sum_constraints_lhs), *this - ); + state, *this, std::move(number_data), std::move(sum_constraints_lhs)); } } @@ -1667,15 +1674,15 @@ void BinaryNode::exchange( if (i_slices.has_value()) { assert(j_slices.has_value()); // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - state_data->update(*this, i, difference, *i_slices); + state_data->update(i, difference, *i_slices); // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - state_data->update(*this, j, -difference, *j_slices); + state_data->update(j, -difference, *j_slices); } else { assert(!j_slices.has_value()); // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - state_data->update(*this, i, difference); + state_data->update(i, difference); // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - state_data->update(*this, j, -difference); + state_data->update(j, -difference); } } } @@ -1695,9 +1702,9 @@ void BinaryNode::clip_and_set_value( // If change occurred and sum constraint exist, update running sums. if (sum_constraints_.size() > 0) { if (slices.has_value()) { - state_data->update(*this, index, value - diff(state).back().old, *slices); + state_data->update(index, value - diff(state).back().old, *slices); } else { - state_data->update(*this, index, value - diff(state).back().old); + state_data->update(index, value - diff(state).back().old); } } } @@ -1720,9 +1727,9 @@ void BinaryNode::set_value( // If change occurred and sum constraint exist, update running sums. if (sum_constraints_.size() > 0) { if (slices.has_value()) { - state_data->update(*this, index, value - diff(state).back().old, *slices); + state_data->update(index, value - diff(state).back().old, *slices); } else { - state_data->update(*this, index, value - diff(state).back().old); + state_data->update(index, value - diff(state).back().old); } } } @@ -1744,9 +1751,9 @@ void BinaryNode::flip( // If value changed from 0 -> 1, update by 1. // If value changed from 1 -> 0, update by -1. if (slices.has_value()) { - state_data->update(*this, index, (state_data->get(index) == 1) ? 1 : -1, *slices); + state_data->update(index, (state_data->get(index) == 1) ? 1 : -1, *slices); } else { - state_data->update(*this, index, (state_data->get(index) == 1) ? 1 : -1); + state_data->update(index, (state_data->get(index) == 1) ? 1 : -1); } } } From b0f6c0711d843b646a1ef20b5c1a0fe7b05bfd27 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Fri, 7 Aug 2026 14:47:40 -0700 Subject: [PATCH 17/21] Make a single (virtual) overload for NumberNodeStateData::update() --- dwave/optimization/src/nodes/numbers.cpp | 199 +++++++++++++---------- 1 file changed, 112 insertions(+), 87 deletions(-) diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index 42731c61..baad8f53 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -195,13 +195,11 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat /// Update the relevant sum constraints running sums (`lhs`) given that the /// value stored at `index` is changed by `difference`. - void update(const ssize_t index, const double difference); - /// Users may pass the slices (per sum constraint) that `index` lies on. - void update( - const ssize_t index, - const double difference, - std::vector slices + virtual void update( + ssize_t index, + double difference, + std::optional> optional_slices = std::nullopt ); /// For each sum constraint, track the sum of the values within each slice. @@ -258,60 +256,58 @@ void NumberNodeStateData::revert() { assert(slice_cache_.empty()); } -void NumberNodeStateData::update( - const ssize_t index, - const double difference -) { - const auto& sum_constraints = node_.sum_constraints(); - assert(sum_constraints.size() != 0); // Should only call where applicable. - assert(difference != 0); // Should not call when no change occurs. - assert(sum_constraints.size() == sum_constraints_lhs.size()); - - std::vector cache_entry; // Initialize the slice cache. - cache_entry.reserve(sum_constraints.size()); - // Get multidimensional indices for `index` so we can identify the slices - // `index` lies on per sum constraint. - const std::vector multi_index = unravel_index(index, node_.shape()); - assert(sum_constraints.size() <= multi_index.size()); - // For each sum constraint. - for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { - const std::optional axis = sum_constraints[i].axis(); - /// Determine the "slice" that index lies on given the sum constraint. - /// If `axis == std::nullopt`, the array is treated as a flat array with a - /// single slice. Otherwise, the slice is defined by multi_index. - assert(!axis.has_value() || *axis < static_cast(multi_index.size())); - const ssize_t slice = axis.has_value() ? multi_index[*axis] : 0; - assert(0 <= slice && slice < static_cast(sum_constraints_lhs[i].size())); - sum_constraints_lhs[i][slice] += difference; // Offset slice sum. - cache_entry.push_back(slice); // Record the slice in the cache. - } - slice_cache_.emplace_back(std::move(cache_entry)); // Cache the slices. -} - void NumberNodeStateData::update( const ssize_t index, const double difference, - std::vector slices + std::optional> optional_slices ) { const auto& sum_constraints = node_.sum_constraints(); assert(sum_constraints.size() != 0); // Should only call where applicable. assert(difference != 0); // Should not call when no change occurs. assert(sum_constraints.size() == sum_constraints_lhs.size()); - assert(sum_constraints.size() == slices.size()); - // For each sum constraint. - for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { - // Sanity check that the user provided slices for `index` are correct. - assert(([&]() { + + // Dev note: there is a tonne of deduplication one could do here. Keeping this + // as-is to minimize changes in the current PR. This needs another pass in the + // future. + if (optional_slices) { + std::vector slices = std::move(*optional_slices); + + assert(sum_constraints.size() == slices.size()); + // For each sum constraint. + for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { + // Sanity check that the user provided slices for `index` are correct. + assert(([&]() { + const std::optional axis = sum_constraints[i].axis(); + /// Determine the "slice" that index lies on given the sum constraint. + /// If `axis == std::nullopt`, the array is treated as a flat array with a + /// single slice. Otherwise, the slice is defined by unravel_index(). + if (!axis.has_value()) return slices[i] == 0; + return slices[i] == unravel_index(index, node_.shape())[*axis]; + })()); + sum_constraints_lhs[i][slices[i]] += difference; // Offset slice sum. + } + slice_cache_.emplace_back(std::move(slices)); // Cache the slices. + } else { + std::vector cache_entry; // Initialize the slice cache. + cache_entry.reserve(sum_constraints.size()); + // Get multidimensional indices for `index` so we can identify the slices + // `index` lies on per sum constraint. + const std::vector multi_index = unravel_index(index, node_.shape()); + assert(sum_constraints.size() <= multi_index.size()); + // For each sum constraint. + for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { const std::optional axis = sum_constraints[i].axis(); /// Determine the "slice" that index lies on given the sum constraint. /// If `axis == std::nullopt`, the array is treated as a flat array with a - /// single slice. Otherwise, the slice is defined by unravel_index(). - if (!axis.has_value()) return slices[i] == 0; - return slices[i] == unravel_index(index, node_.shape())[*axis]; - })()); - sum_constraints_lhs[i][slices[i]] += difference; // Offset slice sum. + /// single slice. Otherwise, the slice is defined by multi_index. + assert(!axis.has_value() || *axis < static_cast(multi_index.size())); + const ssize_t slice = axis.has_value() ? multi_index[*axis] : 0; + assert(0 <= slice && slice < static_cast(sum_constraints_lhs[i].size())); + sum_constraints_lhs[i][slice] += difference; // Offset slice sum. + cache_entry.push_back(slice); // Record the slice in the cache. + } + slice_cache_.emplace_back(std::move(cache_entry)); // Cache the slices. } - slice_cache_.emplace_back(std::move(slices)); // Cache the slices. } double const* NumberNode::buff(const State& state) const noexcept { @@ -1447,14 +1443,12 @@ struct BinaryNodeStateData : public NumberNodeStateData { /// Update `sum_constraints_lhs` and `slice_indices` given that the value /// stored at `index` is changed by `difference`. - void update(const ssize_t index, const double difference); - /// Users may pass the slices (per sum constraint) that `index` lies on. void update( - const ssize_t index, - const double difference, - std::vector slices - ); + ssize_t index, + double difference, + std::optional> optional_slices = std::nullopt + ) override; /// A collection of DisjointSparseSet, one per sum constraint. std::vector slice_indices; @@ -1496,15 +1490,78 @@ void BinaryNodeStateData::revert() { ArrayNodeStateData::revert(); // Revert changes to the buffer. } +// void BinaryNodeStateData::update( +// const ssize_t index, +// const double difference +// ) { +// const auto& sum_constraints = node_.sum_constraints(); +// assert(sum_constraints.size() != 0); // Should only call where applicable. +// assert(difference == 1 || difference == -1); +// assert(sum_constraints.size() == sum_constraints_lhs.size()); +// assert(sum_constraints.size() == slice_indices.size()); +// std::vector cache_entry; // Initialize the slice cache. +// cache_entry.reserve(sum_constraints.size()); +// // Get multidimensional indices for `index` so we can identify the slices +// // `index` lies on per sum constraint. +// const std::vector multi_index = unravel_index(index, node_.shape()); +// assert(sum_constraints.size() <= multi_index.size()); +// // For each sum constraint. +// for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { +// const std::optional axis = sum_constraints[i].axis(); +// /// Determine the "slice" that index lies on given the sum constraint. +// /// If `axis == std::nullopt`, the array is treated as a flat array with a +// /// single slice. Otherwise, the slice is defined by multi_index. +// assert(!axis.has_value() || *axis < static_cast(multi_index.size())); +// const ssize_t slice = axis.has_value() ? multi_index[*axis] : 0; +// assert(0 <= slice && slice < static_cast(sum_constraints_lhs[i].size())); +// sum_constraints_lhs[i][slice] += difference; // Offset slice sum. +// // Update tracked indices. +// if (difference == 1.0) { +// slice_indices[i].update_true(index, slice); +// } else { +// slice_indices[i].update_false(index, slice); +// } +// cache_entry.push_back(slice); // Record the slice in the cache. +// } +// slice_cache_.emplace_back(std::move(cache_entry)); // Cache the slices. +// } + void BinaryNodeStateData::update( const ssize_t index, - const double difference + const double difference, + std::optional> optional_slices ) { const auto& sum_constraints = node_.sum_constraints(); assert(sum_constraints.size() != 0); // Should only call where applicable. assert(difference == 1 || difference == -1); assert(sum_constraints.size() == sum_constraints_lhs.size()); assert(sum_constraints.size() == slice_indices.size()); + + if (optional_slices) { + std::vector slices = std::move(*optional_slices); + + assert(sum_constraints.size() == slices.size()); + // For each sum constraint. + for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { + // Sanity check that the user provided slices for `index` are correct. + assert(([&]() { + const std::optional axis = sum_constraints[i].axis(); + /// Determine the "slice" that index lies on given the sum constraint. + /// If `axis == std::nullopt`, the array is treated as a flat array with a + /// single slice. Otherwise, the slice is defined by multi_index. + if (!axis.has_value()) return slices[i] == 0; + return slices[i] == unravel_index(index, node_.shape())[*axis]; + })()); + sum_constraints_lhs[i][slices[i]] += difference; // Offset slice sum. + // Update tracked indices. + if (difference == 1.0) { + slice_indices[i].update_true(index, slices[i]); + } else { + slice_indices[i].update_false(index, slices[i]); + } + } + slice_cache_.emplace_back(std::move(slices)); // Cache the slices. + } else { std::vector cache_entry; // Initialize the slice cache. cache_entry.reserve(sum_constraints.size()); // Get multidimensional indices for `index` so we can identify the slices @@ -1530,39 +1587,7 @@ void BinaryNodeStateData::update( cache_entry.push_back(slice); // Record the slice in the cache. } slice_cache_.emplace_back(std::move(cache_entry)); // Cache the slices. -} - -void BinaryNodeStateData::update( - const ssize_t index, - const double difference, - std::vector slices -) { - const auto& sum_constraints = node_.sum_constraints(); - assert(sum_constraints.size() != 0); // Should only call where applicable. - assert(difference == 1 || difference == -1); - assert(sum_constraints.size() == sum_constraints_lhs.size()); - assert(sum_constraints.size() == slice_indices.size()); - assert(sum_constraints.size() == slices.size()); - // For each sum constraint. - for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { - // Sanity check that the user provided slices for `index` are correct. - assert(([&]() { - const std::optional axis = sum_constraints[i].axis(); - /// Determine the "slice" that index lies on given the sum constraint. - /// If `axis == std::nullopt`, the array is treated as a flat array with a - /// single slice. Otherwise, the slice is defined by multi_index. - if (!axis.has_value()) return slices[i] == 0; - return slices[i] == unravel_index(index, node_.shape())[*axis]; - })()); - sum_constraints_lhs[i][slices[i]] += difference; // Offset slice sum. - // Update tracked indices. - if (difference == 1.0) { - slice_indices[i].update_true(index, slices[i]); - } else { - slice_indices[i].update_false(index, slices[i]); - } } - slice_cache_.emplace_back(std::move(slices)); // Cache the slices. } void BinaryNodeStateData::compute_slice_indices_(const BinaryNode& node) { From aef592b5aa0e62942c1d14f6eeb7b9f96e372a57 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Fri, 7 Aug 2026 15:10:51 -0700 Subject: [PATCH 18/21] Update NumberNode::exhange() to handle subclasses and remove BinaryNode::exhange() --- .../dwave-optimization/nodes/numbers.hpp | 9 -- dwave/optimization/src/nodes/numbers.cpp | 103 +++++++----------- 2 files changed, 37 insertions(+), 75 deletions(-) diff --git a/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp b/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp index b0002ca0..d342961a 100644 --- a/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp +++ b/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp @@ -415,15 +415,6 @@ class BinaryNode : public IntegerNode { return initialize_state(state, std::vector(values.begin(), values.end())); } - /// @copydoc NumberNode::exchange() - void exchange( - State& state, - ssize_t i, - ssize_t j, - std::optional> i_slices = std::nullopt, - std::optional> j_slices = std::nullopt - ) const; - /// @copydoc NumberNode::clip_and_set_value() void clip_and_set_value( State& state, diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index baad8f53..8d3f407a 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -186,6 +186,42 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat assert(slice_cache_.empty()); } + void exchange( + ssize_t i, + ssize_t j, + std::optional> i_slices, + std::optional> j_slices + ) { + // We expect the exchange to obey the index-wise bounds. + assert(node_.lower_bound(i) <= get(j)); + assert(node_.upper_bound(i) >= get(j)); + assert(node_.lower_bound(j) <= get(i)); + assert(node_.upper_bound(j) >= get(i)); + + // assert() that i and j are valid indices occurs in ptr->exchange(). + // State change occurs IFF (i != j) and (buffer[i] != buffer[j]). + if (ArrayNodeStateData::exchange(i, j)) { + // If change occurred and sum constraint exist, update running sums. + if (node_.sum_constraints().size() > 0) { + const double difference = get(i) - get(j); + + if (i_slices.has_value()) { + assert(j_slices.has_value()); + // Index i changed from (what is now) ptr->get(j) to ptr->get(i) + update(i, difference, *i_slices); + // Index j changed from (what is now) ptr->get(i) to ptr->get(j) + update(j, -difference, *j_slices); + } else { + assert(!j_slices.has_value()); + // Index i changed from (what is now) ptr->get(j) to ptr->get(i) + update(i, difference); + // Index j changed from (what is now) ptr->get(i) to ptr->get(j) + update(j, -difference); + } + } + } + } + const NumberNodeCheckpoint_* last_checkpoint() const { return checkpoint_ptr(); } @@ -652,34 +688,7 @@ void NumberNode::exchange( std::optional> i_slices, std::optional> j_slices ) const { - auto state_data = data_ptr_(state); - // We expect the exchange to obey the index-wise bounds. - assert(lower_bound(i) <= state_data->get(j)); - assert(upper_bound(i) >= state_data->get(j)); - assert(lower_bound(j) <= state_data->get(i)); - assert(upper_bound(j) >= state_data->get(i)); - // assert() that i and j are valid indices occurs in ptr->exchange(). - // State change occurs IFF (i != j) and (buffer[i] != buffer[j]). - if (state_data->exchange(i, j)) { - // If change occurred and sum constraint exist, update running sums. - if (sum_constraints_.size() > 0) { - const double difference = state_data->get(i) - state_data->get(j); - - if (i_slices.has_value()) { - assert(j_slices.has_value()); - // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - state_data->update(i, difference, *i_slices); - // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - state_data->update(j, -difference, *j_slices); - } else { - assert(!j_slices.has_value()); - // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - state_data->update(i, difference); - // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - state_data->update(j, -difference); - } - } - } + data_ptr_(state)->exchange(i, j, std::move(i_slices), std::move(j_slices)); } double NumberNode::get_value(const State& state, ssize_t i) const { @@ -1675,44 +1684,6 @@ void BinaryNode::initialize_state(State& state) const { } } -void BinaryNode::exchange( - State& state, - ssize_t i, - ssize_t j, - std::optional> i_slices, - std::optional> j_slices -) const { - auto state_data = data_ptr_(state); - // We expect the exchange to obey the index-wise bounds. - assert(lower_bound(i) <= state_data->get(j)); - assert(upper_bound(i) >= state_data->get(j)); - assert(lower_bound(j) <= state_data->get(i)); - assert(upper_bound(j) >= state_data->get(i)); - // assert() that i and j are valid indices occurs in ptr->exchange(). State - // change occurs IFF (i != j) and (buffer[i] != buffer[j]). - if (state_data->exchange(i, j)) { - // If change occurred and sum constraint exist, update - // running sums. - if (sum_constraints_.size() > 0) { - const double difference = state_data->get(i) - state_data->get(j); - - if (i_slices.has_value()) { - assert(j_slices.has_value()); - // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - state_data->update(i, difference, *i_slices); - // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - state_data->update(j, -difference, *j_slices); - } else { - assert(!j_slices.has_value()); - // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - state_data->update(i, difference); - // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - state_data->update(j, -difference); - } - } - } -} - void BinaryNode::clip_and_set_value( State& state, ssize_t index, From fe7046b4874562f2a7a52b3a8af773c9a16accd4 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Fri, 7 Aug 2026 15:15:33 -0700 Subject: [PATCH 19/21] Add comment to BinaryNodeStateData::update() and remove dead code --- dwave/optimization/src/nodes/numbers.cpp | 87 ++++++++---------------- 1 file changed, 27 insertions(+), 60 deletions(-) diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index 8d3f407a..af7c3984 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -1499,42 +1499,6 @@ void BinaryNodeStateData::revert() { ArrayNodeStateData::revert(); // Revert changes to the buffer. } -// void BinaryNodeStateData::update( -// const ssize_t index, -// const double difference -// ) { -// const auto& sum_constraints = node_.sum_constraints(); -// assert(sum_constraints.size() != 0); // Should only call where applicable. -// assert(difference == 1 || difference == -1); -// assert(sum_constraints.size() == sum_constraints_lhs.size()); -// assert(sum_constraints.size() == slice_indices.size()); -// std::vector cache_entry; // Initialize the slice cache. -// cache_entry.reserve(sum_constraints.size()); -// // Get multidimensional indices for `index` so we can identify the slices -// // `index` lies on per sum constraint. -// const std::vector multi_index = unravel_index(index, node_.shape()); -// assert(sum_constraints.size() <= multi_index.size()); -// // For each sum constraint. -// for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { -// const std::optional axis = sum_constraints[i].axis(); -// /// Determine the "slice" that index lies on given the sum constraint. -// /// If `axis == std::nullopt`, the array is treated as a flat array with a -// /// single slice. Otherwise, the slice is defined by multi_index. -// assert(!axis.has_value() || *axis < static_cast(multi_index.size())); -// const ssize_t slice = axis.has_value() ? multi_index[*axis] : 0; -// assert(0 <= slice && slice < static_cast(sum_constraints_lhs[i].size())); -// sum_constraints_lhs[i][slice] += difference; // Offset slice sum. -// // Update tracked indices. -// if (difference == 1.0) { -// slice_indices[i].update_true(index, slice); -// } else { -// slice_indices[i].update_false(index, slice); -// } -// cache_entry.push_back(slice); // Record the slice in the cache. -// } -// slice_cache_.emplace_back(std::move(cache_entry)); // Cache the slices. -// } - void BinaryNodeStateData::update( const ssize_t index, const double difference, @@ -1546,6 +1510,9 @@ void BinaryNodeStateData::update( assert(sum_constraints.size() == sum_constraints_lhs.size()); assert(sum_constraints.size() == slice_indices.size()); + // Dev note: there is a tonne of deduplication one could do here. Keeping this + // as-is to minimize changes in the current PR. This needs another pass in the + // future. if (optional_slices) { std::vector slices = std::move(*optional_slices); @@ -1571,31 +1538,31 @@ void BinaryNodeStateData::update( } slice_cache_.emplace_back(std::move(slices)); // Cache the slices. } else { - std::vector cache_entry; // Initialize the slice cache. - cache_entry.reserve(sum_constraints.size()); - // Get multidimensional indices for `index` so we can identify the slices - // `index` lies on per sum constraint. - const std::vector multi_index = unravel_index(index, node_.shape()); - assert(sum_constraints.size() <= multi_index.size()); - // For each sum constraint. - for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { - const std::optional axis = sum_constraints[i].axis(); - /// Determine the "slice" that index lies on given the sum constraint. - /// If `axis == std::nullopt`, the array is treated as a flat array with a - /// single slice. Otherwise, the slice is defined by multi_index. - assert(!axis.has_value() || *axis < static_cast(multi_index.size())); - const ssize_t slice = axis.has_value() ? multi_index[*axis] : 0; - assert(0 <= slice && slice < static_cast(sum_constraints_lhs[i].size())); - sum_constraints_lhs[i][slice] += difference; // Offset slice sum. - // Update tracked indices. - if (difference == 1.0) { - slice_indices[i].update_true(index, slice); - } else { - slice_indices[i].update_false(index, slice); + std::vector cache_entry; // Initialize the slice cache. + cache_entry.reserve(sum_constraints.size()); + // Get multidimensional indices for `index` so we can identify the slices + // `index` lies on per sum constraint. + const std::vector multi_index = unravel_index(index, node_.shape()); + assert(sum_constraints.size() <= multi_index.size()); + // For each sum constraint. + for (ssize_t i = 0, stop = static_cast(sum_constraints.size()); i < stop; ++i) { + const std::optional axis = sum_constraints[i].axis(); + /// Determine the "slice" that index lies on given the sum constraint. + /// If `axis == std::nullopt`, the array is treated as a flat array with a + /// single slice. Otherwise, the slice is defined by multi_index. + assert(!axis.has_value() || *axis < static_cast(multi_index.size())); + const ssize_t slice = axis.has_value() ? multi_index[*axis] : 0; + assert(0 <= slice && slice < static_cast(sum_constraints_lhs[i].size())); + sum_constraints_lhs[i][slice] += difference; // Offset slice sum. + // Update tracked indices. + if (difference == 1.0) { + slice_indices[i].update_true(index, slice); + } else { + slice_indices[i].update_false(index, slice); + } + cache_entry.push_back(slice); // Record the slice in the cache. } - cache_entry.push_back(slice); // Record the slice in the cache. - } - slice_cache_.emplace_back(std::move(cache_entry)); // Cache the slices. + slice_cache_.emplace_back(std::move(cache_entry)); // Cache the slices. } } From 7c05cce149344deda8f87e5f2de99101231b4d06 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Fri, 7 Aug 2026 15:39:29 -0700 Subject: [PATCH 20/21] Update NumberNode::set_value() to handle subclasses --- .../dwave-optimization/nodes/numbers.hpp | 45 +--- dwave/optimization/src/nodes/numbers.cpp | 241 +++++++----------- 2 files changed, 101 insertions(+), 185 deletions(-) diff --git a/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp b/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp index d342961a..ea4c18f2 100644 --- a/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp +++ b/dwave/optimization/include/dwave-optimization/nodes/numbers.hpp @@ -121,6 +121,10 @@ class NumberNode : public ArrayOutputMixin, public DecisionNode { // NumberNode methods ***************************************************** + /// @copydoc DecisionNode::assign_from_checkpoint() + void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const override; + void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const override; + /// @copydoc DecisionNode::checkpoint() checkpoint_type checkpoint(State& state) const override; @@ -155,6 +159,15 @@ class NumberNode : public ArrayOutputMixin, public DecisionNode { std::optional> slices = std::nullopt ) const; + // Set the value at the given index in the given state. + // Users may pass the slices (per sum constraint) that each index lies on. + void set_value( + State& state, + ssize_t index, + double value, + std::optional> slices = std::nullopt + ) const; + /// Return the stateless sum constraints. const std::vector& sum_constraints() const; @@ -290,20 +303,6 @@ class IntegerNode : public NumberNode { // @copydoc NumberNode::is_valid() bool is_valid(ssize_t index, double value) const override; - // IntegerNode methods **************************************************** - - /// @copydoc DecisionNode::assign_from_checkpoint() - void assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const override; - void assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const override; - - // Set the value at the given index in the given state. - // Users may pass the slices (per sum constraint) that each index lies on. - void set_value( - State& state, - ssize_t index, - double value, - std::optional> slices = std::nullopt - ) const; protected: // Overloads needed by the Node ABC *************************************** @@ -415,24 +414,6 @@ class BinaryNode : public IntegerNode { return initialize_state(state, std::vector(values.begin(), values.end())); } - /// @copydoc NumberNode::clip_and_set_value() - void clip_and_set_value( - State& state, - ssize_t index, - double value, - std::optional> slices = std::nullopt - ) const; - - /// ** Redefined IntegerNode method since BinaryNode has custom StateData ** - - /// @copydoc IntegerNode::set_value() - void set_value( - State& state, - ssize_t index, - double value, - std::optional> slices = std::nullopt - ) const; - /// ************************** BinaryNode methods ************************** // Flip the value (0 -> 1 or 1 -> 0) at `index` in the given state. diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index af7c3984..49815fbb 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -229,6 +229,30 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat /// Revert the state dependent data of NumberNode. void revert(); + void set( + ssize_t index, + double value, + std::optional> slices + ) { + // We expect `value` to obey the index-wise bounds and integrality + assert(node_.lower_bound(index) <= value); + assert(node_.upper_bound(index) >= value); + assert(not node_.integral() or value == std::round(value)); + + // assert() that i is a valid index occurs in ptr->set(). + // State change occurs IFF `value` != buffer[index]. + if (ArrayNodeStateData::set(index, value)) { + // If change occurred and sum constraint exist, update running sums. + if (node_.sum_constraints().size() > 0) { + if (slices.has_value()) { + update(index, value - diff().back().old, *slices); + } else { + update(index, value - diff().back().old); + } + } + } + } + /// Update the relevant sum constraints running sums (`lhs`) given that the /// value stored at `index` is changed by `difference`. /// Users may pass the slices (per sum constraint) that `index` lies on. @@ -346,6 +370,56 @@ void NumberNodeStateData::update( } } +void NumberNode::assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const { + auto state_data = data_ptr_(state); + + auto* checkpoint_ptr = static_cast(checkpoint.get()); + + assert(checkpoint_ptr == state_data->last_checkpoint()); + + // Check if there are any changes not otherwise tracked by a checkpoint that we need + // to revert first. + // A better way would be to implement a partial revert on our state class, but this + // is not a path we care about greatly so let's err on the side of simple and well- + // tested. + if (ssize_t excess_updates = state_data->diff().size() - checkpoint_ptr->drop()) { + assert(excess_updates > 0); + + for ( + const auto& [idx, old, _] : + state_data->diff() | std::views::reverse | std::views::take(excess_updates) + ) { + state_data->set(idx, old, std::nullopt); + } + } + + auto [updates, optional_slice_cache] = checkpoint_ptr->detach_updates(); + + if (optional_slice_cache.has_value()) { + assert(sum_constraints_.size() > 0); + + auto slices_rit = std::ranges::rbegin(*optional_slice_cache); + + for (const auto& [idx, old, _] : std::move(updates) | std::views::reverse) { + state_data->set(idx, old, *(slices_rit++)); + } + } else { + assert(updates.empty() or sum_constraints_.empty()); + + // in this case we don't need to do anything to update the slice data + for (const auto& [idx, old, _] : std::move(updates) | std::views::reverse) { + state_data->set(idx, old, std::nullopt); + } + } + + checkpoint_ptr->drop() = state_data->diff().size(); +} + +void NumberNode::assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const { + assign_from_checkpoint(state, checkpoint); // call the lvalue version + checkpoint.reset(); +} + double const* NumberNode::buff(const State& state) const noexcept { return data_ptr_(state)->buff(); } @@ -737,20 +811,18 @@ void NumberNode::clip_and_set_value( double value, std::optional> slices ) const { - auto state_data = data_ptr_(state); - value = std::clamp(value, lower_bound(index), upper_bound(index)); - // assert() that i is a valid index occurs in ptr->set(). - // State change occurs IFF `value` != buffer[index]. - if (state_data->set(index, value)) { - // If change occurred and sum constraint exist, update running sums. - if (sum_constraints_.size() > 0) { - if (slices.has_value()) { - state_data->update(index, value - diff(state).back().old, *slices); - } else { - state_data->update(index, value - diff(state).back().old); - } - } - } + data_ptr_(state)->set( + index, std::clamp(value, lower_bound(index), upper_bound(index)), std::move(slices) + ); +} + +void NumberNode::set_value( + State& state, + ssize_t index, + double value, + std::optional> slices +) const { + data_ptr_(state)->set(index, value, std::move(slices)); } const std::vector& NumberNode::sum_constraints() const { @@ -1070,59 +1142,6 @@ IntegerNode::IntegerNode( std::move(sum_constraints) ) {} -void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type& checkpoint) const { - auto state_data = data_ptr_(state); - - auto* checkpoint_ptr = static_cast(checkpoint.get()); - - assert(checkpoint_ptr == state_data->last_checkpoint()); - - // Check if there are any changes not otherwise tracked by a checkpoint that we need - // to revert first. - // A better way would be to implement a partial revert on our state class, but this - // is not a path we care about greatly so let's err on the side of simple and well- - // tested. - if (ssize_t excess_updates = state_data->diff().size() - checkpoint_ptr->drop()) { - assert(excess_updates > 0); - - for ( - const auto& [idx, old, _] : - state_data->diff() | std::views::reverse | std::views::take(excess_updates) - ) { - // This is a *very* expensive call. But, again, we're not too worried about - // performance here. - this->set_value(state, idx, old); - } - } - - auto [updates, optional_slice_cache] = checkpoint_ptr->detach_updates(); - - if (optional_slice_cache.has_value()) { - assert(sum_constraints_.size() > 0); - - auto slices_rit = std::ranges::rbegin(*optional_slice_cache); - - for (const auto& [idx, old, _] : std::move(updates) | std::views::reverse) { - state_data->set(idx, old); - state_data->update(idx, old - diff(state).back().old, *(slices_rit++)); - } - } else { - assert(updates.empty() or sum_constraints_.empty()); - - // in this case we don't need to do anything to update the slice data - for (const auto& [idx, old, _] : std::move(updates) | std::views::reverse) { - state_data->set(idx, old); - } - } - - checkpoint_ptr->drop() = state_data->diff().size(); -} - -void IntegerNode::assign_from_checkpoint(State& state, checkpoint_type&& checkpoint) const { - assign_from_checkpoint(state, checkpoint); // call the lvalue version - checkpoint.reset(); -} - bool IntegerNode::integral() const { return true; } bool IntegerNode::is_valid(ssize_t index, double value) const { @@ -1130,31 +1149,6 @@ bool IntegerNode::is_valid(ssize_t index, double value) const { (std::round(value) == value); } -void IntegerNode::set_value( - State& state, - ssize_t index, - double value, - std::optional> slices -) const { - auto state_data = data_ptr_(state); - // We expect `value` to obey the index-wise bounds and to be an integer. - assert(lower_bound(index) <= value); - assert(upper_bound(index) >= value); - assert(value == std::round(value)); - // assert() that i is a valid index occurs in ptr->set(). - // State change occurs IFF `value` != buffer[index]. - if (state_data->set(index, value)) { - // If change occurred and sum constraint exist, update running sums. - if (sum_constraints_.size() > 0) { - if (slices.has_value()) { - state_data->update(index, value - diff(state).back().old, *slices); - } else { - state_data->update(index, value - diff(state).back().old); - } - } - } -} - double IntegerNode::default_value(ssize_t index) const { return (lower_bound(index) <= 0 && upper_bound(index) >= 0) ? 0 : lower_bound(index); } @@ -1651,53 +1645,6 @@ void BinaryNode::initialize_state(State& state) const { } } -void BinaryNode::clip_and_set_value( - State& state, - ssize_t index, - double value, - std::optional> slices -) const { - auto state_data = data_ptr_(state); - value = std::clamp(value, lower_bound(index), upper_bound(index)); - // assert() that i is a valid index occurs in ptr->set(). - // State change occurs IFF `value` != buffer[index]. - if (state_data->set(index, value)) { - // If change occurred and sum constraint exist, update running sums. - if (sum_constraints_.size() > 0) { - if (slices.has_value()) { - state_data->update(index, value - diff(state).back().old, *slices); - } else { - state_data->update(index, value - diff(state).back().old); - } - } - } -} - -void BinaryNode::set_value( - State& state, - ssize_t index, - double value, - std::optional> slices -) const { - auto state_data = data_ptr_(state); - // We expect `value` to obey the index-wise bounds and to be an integer. - assert(lower_bound(index) <= value); - assert(upper_bound(index) >= value); - assert(value == std::round(value)); - // assert() that i is a valid index occurs in ptr->set(). - // State change occurs IFF `value` != buffer[index]. - if (state_data->set(index, value)) { - // If change occurred and sum constraint exist, update running sums. - if (sum_constraints_.size() > 0) { - if (slices.has_value()) { - state_data->update(index, value - diff(state).back().old, *slices); - } else { - state_data->update(index, value - diff(state).back().old); - } - } - } -} - void BinaryNode::flip( State& state, ssize_t index, @@ -1706,20 +1653,8 @@ void BinaryNode::flip( auto state_data = data_ptr_(state); // Variable should not be fixed. assert(lower_bound(index) != upper_bound(index)); - // assert() that `index` is valid occurs in ptr->set(). - // State change occurs IFF `value` != buffer[index]. - if (state_data->set(index, !state_data->get(index))) { - // If change occurred and sum constraint exist, update running sums. - if (sum_constraints_.size() > 0) { - // If value changed from 0 -> 1, update by 1. - // If value changed from 1 -> 0, update by -1. - if (slices.has_value()) { - state_data->update(index, (state_data->get(index) == 1) ? 1 : -1, *slices); - } else { - state_data->update(index, (state_data->get(index) == 1) ? 1 : -1); - } - } - } + + state_data->set(index, not state_data->get(index), std::move(slices)); } ssize_t BinaryNode::num_true( From 8bf618ffcdc824b6112110b5e04d0cee8979d28d Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Fri, 7 Aug 2026 15:45:55 -0700 Subject: [PATCH 21/21] Make NumberNodeStateData::update() private --- dwave/optimization/src/nodes/numbers.cpp | 51 ++++++++++++------------ 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/dwave/optimization/src/nodes/numbers.cpp b/dwave/optimization/src/nodes/numbers.cpp index 49815fbb..85855051 100644 --- a/dwave/optimization/src/nodes/numbers.cpp +++ b/dwave/optimization/src/nodes/numbers.cpp @@ -208,15 +208,15 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat if (i_slices.has_value()) { assert(j_slices.has_value()); // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - update(i, difference, *i_slices); + update_(i, difference, *i_slices); // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - update(j, -difference, *j_slices); + update_(j, -difference, *j_slices); } else { assert(!j_slices.has_value()); // Index i changed from (what is now) ptr->get(j) to ptr->get(i) - update(i, difference); + update_(i, difference); // Index j changed from (what is now) ptr->get(i) to ptr->get(j) - update(j, -difference); + update_(j, -difference); } } } @@ -245,23 +245,14 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat // If change occurred and sum constraint exist, update running sums. if (node_.sum_constraints().size() > 0) { if (slices.has_value()) { - update(index, value - diff().back().old, *slices); + update_(index, value - diff().back().old, *slices); } else { - update(index, value - diff().back().old); + update_(index, value - diff().back().old); } } } } - /// Update the relevant sum constraints running sums (`lhs`) given that the - /// value stored at `index` is changed by `difference`. - /// Users may pass the slices (per sum constraint) that `index` lies on. - virtual void update( - ssize_t index, - double difference, - std::optional> optional_slices = std::nullopt - ); - /// For each sum constraint, track the sum of the values within each slice. /// `sum_constraints_lhs[i][j]` is the sum of the values within the `j`th slice /// along the `axis`* defined by the `i`th sum constraint. @@ -281,6 +272,16 @@ class NumberNodeStateData : public ArrayNodeStateData, public CheckpointableStat /// Hold a reference to the parent node. This class can outlive the node but /// cannot be accessed except through it. const NumberNode& node_; + + private: + /// Update the relevant sum constraints running sums (`lhs`) given that the + /// value stored at `index` is changed by `difference`. + /// Users may pass the slices (per sum constraint) that `index` lies on. + virtual void update_( + ssize_t index, + double difference, + std::optional> optional_slices = std::nullopt + ); }; void NumberNodeStateData::revert() { @@ -316,7 +317,7 @@ void NumberNodeStateData::revert() { assert(slice_cache_.empty()); } -void NumberNodeStateData::update( +void NumberNodeStateData::update_( const ssize_t index, const double difference, std::optional> optional_slices @@ -1444,21 +1445,21 @@ struct BinaryNodeStateData : public NumberNodeStateData { /// Revert the state dependent data of BinaryNode. void revert(); + /// A collection of DisjointSparseSet, one per sum constraint. + std::vector slice_indices; + + private: + /// Populate `slice_indices` given the BinaryNode and its assigned values. + void compute_slice_indices_(const BinaryNode& node); + /// Update `sum_constraints_lhs` and `slice_indices` given that the value /// stored at `index` is changed by `difference`. /// Users may pass the slices (per sum constraint) that `index` lies on. - void update( + void update_( ssize_t index, double difference, std::optional> optional_slices = std::nullopt ) override; - - /// A collection of DisjointSparseSet, one per sum constraint. - std::vector slice_indices; - - private: - /// Populate `slice_indices` given the BinaryNode and its assigned values. - void compute_slice_indices_(const BinaryNode& node); }; void BinaryNodeStateData::revert() { @@ -1493,7 +1494,7 @@ void BinaryNodeStateData::revert() { ArrayNodeStateData::revert(); // Revert changes to the buffer. } -void BinaryNodeStateData::update( +void BinaryNodeStateData::update_( const ssize_t index, const double difference, std::optional> optional_slices