Add method for removing redundant nodes by arcondello · Pull Request #571 · dwavesystems/dwave-optimization

arcondello · 2026-06-24T15:38:40Z

Closes #563

AI Generation Disclosure

No AI used.

In C++20 Node::operator==(const Node&) const is amibiguous because either order is valid.

This requires some fiddling, see https://isocpp.org/wiki/faq/templates#nondependent-name-lookup-members for the why.

arcondello · 2026-07-06T16:57:26Z

+    virtual bool equal_to(const Derived& rhs) const = 0;
+};
+
+/// Overload for EqualityMixin that does not require a class-specific equal_to()


A doubt I have is whether these two overloads are different enough that they should actually be given different names 🤔

arcondello · 2026-07-10T17:55:58Z

As a note: I didn't try to be inclusive about the asserts in the replace_predecessor_() method. That way lies madness so I just went with some sanity checks.

fastbodin

Managed to work through all files excluding the test files. Thought I would upload these comments before moving onto them.

fastbodin · 2026-07-13T17:00:12Z

+            >>> model = Model()
+            ...
+            >>> x = model.integer(5)
+            ...


Nit: I find that the ... do not add much to the example. Additionally, most of the other examples do not include .... Am I missing something?

fastbodin · 2026-07-13T17:39:48Z

+            if (not dropped(uptr)) continue;
+
+            // Remove the node from its predecessor's successor vectors.
+            // This very leaves the node in an invalid state, very briefly


Suggested change

// This very leaves the node in an invalid state, very briefly

// This leaves the node in an invalid state, very briefly

fastbodin · 2026-07-13T20:20:57Z

+
+        index -= 1;


// If we haven't found the Array* we want to replace, then "normalize" // the index so that it looks like it would if `initial_` wasn't an array.

fastbodin · 2026-07-13T20:37:31Z

+bool UnaryOpNode<UnaryOp>::equal_to(const Node& rhs) const {
+    const auto* rhs_ptr = dynamic_cast<const UnaryOpNode*>(&rhs);
+    if (rhs_ptr == nullptr) return false;  // not same type so not equal
+    return this->equal_to(*rhs_ptr);


Confirming that I am not sleeping at the wheel. I had to do a double take here 😆

Probably should change 190 to

const UnaryOpNode* rhs_ptr = dynamic_cast<const UnaryOpNode*>(&rhs);

to make it a little clearer what's happening.

Usually I am fine with auto here, but, in this case, that would actually be helpful.

arcondello · 2026-07-14T16:25:37Z

        }
    }
+
+    SECTION("predecessor replacement") {


For some nodes that use the EqualityMixin without needing to implement another overload I stopped bothering to write separate tests for them, reasoning that it is already extremely well covered elsewhere.

That is fine with me. 👍 You were correct in predicting that I would make such a comment 🤣

fastbodin

I have now gone through all the files. I would like to go through graph.cpp again.

The patience you had when writing these tests is a marvel.

fastbodin · 2026-07-13T22:32:01Z

                graph.emplace_node<ArrayValidationNode>(sets.at(i));
            }

+            THEN("node equality works as expected") {


Test for DisjointListNode too?

fastbodin · 2026-07-13T22:44:24Z

+        Node* a_ptr = graph.emplace_node<BSplineNode>(x0_ptr, k, t0, c0);
+        Node* b_ptr = graph.emplace_node<BSplineNode>(x0_ptr, k, t0, c0);
+        Node* c_ptr = graph.emplace_node<BSplineNode>(x1_ptr, k, t1, c0);
+        Node* d_ptr = graph.emplace_node<BSplineNode>(x0_ptr, k, t0, c1);


Just because BSplineNode::equal_to() also checks k, I would add

Node* e_ptr = graph.emplace_node<BSplineNode>(x0_ptr, 3, t0, c0);

and the corresponding checks like

CHECK(not a_ptr->equal_to(*e_ptr));

fastbodin · 2026-07-14T16:26:51Z

+void ARangeNode::replace_predecessor_(ssize_t index, Node* node_ptr) {
+    Node::replace_predecessor_(index, node_ptr);
+
+    assert(0 <= index and static_cast<size_t>(index) < predecessors().size());


This logic makes sense. Consider me convinced.

fastbodin · 2026-07-14T16:28:27Z

        """
        return self.num_nodes()

+    def remove_redundant_symbols(self, time_limit_s=None):


Worth adding a note in here that this is ineffective on named symbols?

fastbodin · 2026-07-14T16:38:27Z

+        auto* c0_ptr = graph.emplace_node<ConstantNode>(std::vector{0, 1, 2, 3});
+        auto* c1_ptr = graph.emplace_node<ConstantNode>(std::vector{7, 6, 5, 4});
+
+        auto* argsort_ptr = graph.emplace_node<MeanNode>(c0_ptr);


fastbodin · 2026-07-14T16:38:49Z

+        auto* c0_ptr = graph.emplace_node<ConstantNode>(std::vector{0, 1, 2, 3});
+        auto* c1_ptr = graph.emplace_node<ConstantNode>(std::vector{7, 6, 5, 4});
+
+        auto* argsort_ptr = graph.emplace_node<UnaryOpNode<TestType>>(c0_ptr);


unaryop_ptr

fastbodin · 2026-07-14T16:39:45Z

+        auto* y_ptr = graph.emplace_node<BinaryNode>();
+
+        auto* left_x_plus_y = graph.emplace_node<AddNode>(x_ptr, y_ptr);
+        auto* right_x_plus_y = graph.emplace_node<AddNode>(x_ptr, y_ptr);


Suggested change

auto* right_x_plus_y = graph.emplace_node<AddNode>(x_ptr, y_ptr);

auto* right_x_plus_y = graph.emplace_node<AddNode>(y_ptr, x_ptr);

As per the variable name?

arcondello added the enhancement New feature or request label Jun 24, 2026

arcondello force-pushed the feature/node-equality-and-successor-transfer branch 2 times, most recently from 4dab596 to a8040b8 Compare June 24, 2026 18:09

Add methods to remove redundant nodes

82cc356

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from a8040b8 to 82cc356 Compare June 24, 2026 18:14

Add BinaryOpNode equality and predecessor replacement

57638a8

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from 56195c1 to 57638a8 Compare June 24, 2026 18:18

arcondello added 7 commits June 24, 2026 13:32

Switch from Node::operator== to Node::equal_to

16a2938

In C++20 Node::operator==(const Node&) const is amibiguous because either order is valid.

Add ARangeNode equality and predecessor replacement

c0e52bc

Add flow nodes equality and predecessor replacement

948c199

Add indexing nodes equality and predecessor replacement

0443585

Add InputNode equality and predecessor replacement

2985dcf

Add interpolation node equality and predecessor replacement

1cece38

Add lambda node equality and predecessor replacement

055fe62

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from 39fb39d to 055fe62 Compare July 2, 2026 17:35

Add EqualityMixin to reduce code duplication

770a9cb

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from ee71c02 to 770a9cb Compare July 3, 2026 17:05

Use EqualityMixin with BinaryOpNode

5b213bc

This requires some fiddling, see https://isocpp.org/wiki/faq/templates#nondependent-name-lookup-members for the why.

arcondello commented Jul 3, 2026

View reviewed changes

Comment thread dwave/optimization/include/dwave-optimization/nodes/binaryop.hpp

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from 261d2d6 to 98e4bd0 Compare July 3, 2026 18:09

Add node equality and predecessor replacement to MatrixMultiplyNode

47e1be7

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from 98e4bd0 to 47e1be7 Compare July 3, 2026 18:14

Add node equality and predecessor replacement for LP nodes

cd107a0

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from 90386b6 to cd107a0 Compare July 6, 2026 16:43

arcondello commented Jul 6, 2026

View reviewed changes

arcondello added 3 commits July 8, 2026 10:06

Add equality and predecessor replacement for manipulation nodes

6915ba4

Add equality and predecessor replacement for nary op nodes

edf9a68

Add equality for QuadraticModelNode

f7a75d1

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from 42a0edd to f7a75d1 Compare July 8, 2026 17:33

Add equality and predecessor replacement for ReduceNode

cf7f812

arcondello commented Jul 9, 2026

View reviewed changes

Comment thread dwave/optimization/src/nodes/lp.cpp Outdated

arcondello added 2 commits July 9, 2026 13:25

Add equality and predecessor replacement for IsInNode

b1df040

Add equality and predecessor replacement to SoftMaxNode

f643bca

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from 4e887e4 to f643bca Compare July 9, 2026 22:40

arcondello added 5 commits July 9, 2026 15:45

Add equality and predecessor replacement to ArgSortNode

08728d9

Add equality and predecessor replacement to MeanNode

7e3ccae

Add equality and predecessor replacement to unary op nodes

918d197

Make Node::equal_to() and abstract method

b43d9d3

Fix misc issues with redundant_node PR

c60bcb7

arcondello marked this pull request as ready for review July 10, 2026 17:45

arcondello commented Jul 10, 2026

View reviewed changes

Comment thread tests/cpp/test_graph.cpp Outdated

arcondello requested a review from fastbodin July 10, 2026 18:27

Fix objective handling in Graph::remove_redundant_nodes()

d0dd3d3

arcondello force-pushed the feature/node-equality-and-successor-transfer branch from 212caf4 to d0dd3d3 Compare July 10, 2026 19:05

arcondello added 2 commits July 10, 2026 12:11

Add a test for removing redundant constants

a7912dd

Add a smoke test for Model.remove_redundant_symbols()

3583a42

fastbodin reviewed Jul 13, 2026

View reviewed changes

arcondello commented Jul 14, 2026

View reviewed changes

fastbodin reviewed Jul 14, 2026

View reviewed changes

	// This very leaves the node in an invalid state, very briefly
	// This leaves the node in an invalid state, very briefly

	auto* right_x_plus_y = graph.emplace_node<AddNode>(x_ptr, y_ptr);
	auto* right_x_plus_y = graph.emplace_node<AddNode>(y_ptr, x_ptr);

Uh oh!

Conversation

arcondello commented Jun 24, 2026

AI Generation Disclosure

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arcondello commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fastbodin left a comment

Choose a reason for hiding this comment

Uh oh!

fastbodin Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fastbodin Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fastbodin Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fastbodin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arcondello commented Jul 10, 2026 •

edited

Loading

fastbodin Jul 13, 2026 •

edited

Loading

fastbodin Jul 13, 2026 •

edited

Loading

fastbodin Jul 14, 2026 •

edited

Loading