Add NaryReduceNode and InputNode #166

wbernoudy · 2024-11-14T09:12:25Z

Adds InputNode which subclasses ConstantNode but allows assigning of the state after initialization.

This is to support the new NaryReduceNode, which does an n-ary reduce operation on an arbitrary number of inputs/predecessors. The node is constructed by providing an expression, formulated as a separate Graph, and initial values for the reduction operation.

The current design accepts any Graph but then throws exceptions if it contains unsupported nodes. It would be best if we can pass along helpful error messages to users at the python level. Currently this works by encoding the error message and a Node* in a simple JSON string that is then parsed from Python, where we can grab the corresponding symbol.

wbernoudy · 2024-11-15T07:10:58Z

I tried out some more complicated things to support custom exception handling in Cython. I put it in a separate branch for now as I'm not sure we'll go that direction. Separate PR here to try to make it a bit easier to review wbernoudy#1

arcondello

Mostly looked at design rather than line-by-line review.

dwave/optimization/src/array.cpp

dwave/optimization/include/dwave-optimization/nodes/lambda.hpp

dwave/optimization/include/dwave-optimization/nodes/constants.hpp

arcondello · 2024-11-15T16:03:55Z

dwave/optimization/include/dwave-optimization/nodes/lambda.hpp

+class NaryReduceNode : public ArrayOutputMixin<ArrayNode> {
+ public:
+    // Runtime constructor that can be used from Cython/Python
+    NaryReduceNode(Graph&& expression, const std::vector<InputNode*>& inputs,


I wonder if it makes more sense to not have the user provide inputs etc explicitly, but rather to inspect the expression. Maybe set_objetive() sets the output? And we can traverse the nodes looking for InputNode

I think inputs should be provided explicitly because they need to be mapped to the operands/predecessors. Otherwise we are relying on the implicit ordering of when the nodes are added to the expression model, and it becomes less clear which one is the "special" input that takes the previous value.

But I still find this interface confusing... was thinking about taking a map between inputs/operands as well.

At the Python level this could perhaps be addressed by not adding an input() method, but having a subclass of Model that requires num_inputs at construction-time. Then we rely on the order? We might have to treat inputs as decision variables in that they get topologically ordered immediately in that case... Needs thought.

I like that. The ExpressionModel or whatever (subclass of Model) could disallow adding decisions.

Perhaps we could also somehow pull a list of supported nodes from C++ and then disallow adding unsupported symbols to ExpressionModel. This will go a long way to prevent constructing unsupported expressions, as I don't think we will have any supported nodes that can have non-scalar output given all scalar inputs.

And I'll note I expect the standard flow when creating expressions is just to create a list of Inputs anyway and use that. So exp = ExpressionModel(7), exp.inputs[0] + exp.inputs[1] feels natural to me.

Given that we're now using objective for the output, and expression.input() to create the inputs, IMO we should just read the inputs() and objective() off the expression rather than providing them explicitly. We can always allow reorderings etc via a method like this later.

dwave/optimization/include/dwave-optimization/utils.hpp

dwave/optimization/include/dwave-optimization/nodes/constants.hpp

dwave/optimization/include/dwave-optimization/nodes/lambda.hpp

dwave/optimization/include/dwave-optimization/utils.hpp

dwave/optimization/model.py

dwave/optimization/src/nodes/constants.cpp

dwave/optimization/src/nodes/lambda.cpp

dwave/optimization/include/dwave-optimization/array.hpp

dwave/optimization/include/dwave-optimization/nodes/lambda.hpp

dwave/optimization/src/array.cpp

dwave/optimization/src/nodes/lambda.cpp

arcondello · 2024-12-10T17:31:40Z

dwave/optimization/include/dwave-optimization/graph.hpp

@@ -73,6 +74,8 @@ class Graph {
 public:
    Graph();
    ~Graph();
+    Graph(Graph&&);
+    Graph& operator=(Graph&& other) = default;


For consistency, we should put this in the .cpp with the others. Or move the others into the .hpp.

Nevermind! See #184

arcondello · 2024-12-11T20:08:39Z

dwave/optimization/_model.pyx

@@ -185,7 +186,8 @@ cdef class _Graph:
                objective_id = json.loads(objective_buff)
                if not isinstance(objective_id, int) or objective_id >= model.num_nodes():
                    raise ValueError("objective must be an integer and a valid node id")
-                model.minimize(symbol_from_ptr(model, model._graph.nodes()[objective_id].get()))
+                model._set_objective(symbol_from_ptr(model, model._graph.nodes()[objective_id].get()))
+                # model.minimize(symbol_from_ptr(model, model._graph.nodes()[objective_id].get()))


leftover comment

arcondello · 2024-12-11T20:08:54Z

dwave/optimization/_model.pyx

+            self.lock()
+            self.into_file(file, max_num_states=max_num_states, only_decision=only_decision)
+            self.unlock()
+            return


Why this change?

The context manager version of .lock() is defined only for Model currently. I could also add it to Expression or _Graph.

Ah, excellent point! In which case we should at least do it in a

self.lock() try: self.into_file(...) finally: self.unlock()

Yep, that's better!

dwave/optimization/_model.pyx

arcondello · 2024-12-11T20:10:50Z

dwave/optimization/_model.pyx

-            >>> model.minimize(y)
-        """
+    def _set_objective(self, ArraySymbol value):
+        """Set the objective value on the ``dwave::optimization::Graph``."""


This might be a good place to note that we are using "objective" somewhat generously, and that it can refer to the output of an expression as well.

dwave/optimization/_model.pyx

arcondello · 2024-12-11T20:14:07Z

dwave/optimization/include/dwave-optimization/nodes/lambda.hpp

+class NaryReduceNode : public ArrayOutputMixin<ArrayNode> {
+ public:
+    // Runtime constructor that can be used from Cython/Python
+    NaryReduceNode(Graph&& expression, const std::vector<InputNode*>& inputs,


Given that we're now using objective for the output, and expression.input() to create the inputs, IMO we should just read the inputs() and objective() off the expression rather than providing them explicitly. We can always allow reorderings etc via a method like this later.

dwave/optimization/include/dwave-optimization/utils.hpp

dwave/optimization/libcpp/graph.pxd

dwave/optimization/model.py

dwave/optimization/libcpp/nodes.pxd

dwave/optimization/symbols.pyx

dwave/optimization/_model.pyi

dwave/optimization/include/dwave-optimization/nodes/inputs.hpp

dwave/optimization/symbols.pyx

tests/cpp/nodes/test_constants.cpp

arcondello · 2024-12-16T16:28:23Z

dwave/optimization/symbols.pyx

+    `NaryReduce`).
+
+    Args:
+        expression:


I just noticed that numpy.ufunc.reduce(), functools.reduce() and std::accumulate() all use the left-most argument for the init/accumulated value. So I think we should follow that convention by making it the first input.

For completeness std::reduce() requires commutative and associative, which we definitely don't want to require.

* use span instead of vector in a few places where appropriate * fix bugs found in code review

and call in relevant methods

arcondello reviewed Nov 15, 2024

View reviewed changes

arcondello added the enhancement New feature or request label Nov 18, 2024

wbernoudy force-pushed the feature/nary-reduce-node branch from fb3ba6f to 15a5390 Compare November 20, 2024 21:23

wbernoudy mentioned this pull request Dec 2, 2024

Separate out Model class into cython _GraphManager and python Model #174

Closed

2 tasks

arcondello mentioned this pull request Dec 4, 2024

Make Model a Python class by moving Cython parts into a new _Graph class #179

Merged

wbernoudy force-pushed the feature/nary-reduce-node branch 3 times, most recently from d3c4f79 to aacee34 Compare December 5, 2024 21:20

arcondello reviewed Dec 5, 2024

View reviewed changes

wbernoudy marked this pull request as ready for review December 11, 2024 18:30

arcondello reviewed Dec 11, 2024

View reviewed changes

arcondello reviewed Dec 12, 2024

View reviewed changes

dwave/optimization/libcpp/nodes.pxd Outdated Show resolved Hide resolved

arcondello reviewed Dec 12, 2024

View reviewed changes

dwave/optimization/symbols.pyx Outdated Show resolved Hide resolved

wbernoudy force-pushed the feature/nary-reduce-node branch from 69bbd65 to be9f5ee Compare December 13, 2024 04:07

arcondello reviewed Dec 13, 2024

View reviewed changes

arcondello approved these changes Dec 13, 2024

View reviewed changes

arcondello reviewed Dec 16, 2024

View reviewed changes

wbernoudy force-pushed the feature/nary-reduce-node branch from be65ccc to 23cefb1 Compare December 23, 2024 17:54

wbernoudy mentioned this pull request Jan 6, 2025

Mark Decision::update() and ConstantNode::update() as noreturn #189

Merged

wbernoudy added 9 commits January 10, 2025 11:24

Add Expression, InputNode, NaryReduceNode

fbdf5e7

Add docstrings

f8a50cb

Split out InputNode into separate files

7d670bf

* use span instead of vector in a few places where appropriate * fix bugs found in code review

Support Input/NaryReduce serialization

d92428c

Change _Graph.minimize to set_objective

af21f66

and call in relevant methods

Test Input symbol serialization

0ae49c9

Better domain checking for NaryReduce expressions

bd05dcf

Fix NaryReduce bounds tests and check output bounds

0468e9f

Change .input() defaults, fix Expression serialization

1a20ba3

arcondello and others added 11 commits January 10, 2025 11:25

Only define InputNode in one .pxd file

c5ab958

Add more docstrings, fix docstring tests

7ac4d11

Simplify NaryReduceNode constructor

63f3d09

Ensure model is unlocked after .into_file

4c3a50d

Docstring cleanup

94a10b2

Rearrange Input arguments to match other nodes

2f05783

Add release note

e6d438a

Cleanup imports

c614850

Switch reduction val to first input of expression in NaryReduce

92022b8

Expose validate_expression

e89ff75

Expose validation functions for NaryReduceNode

f60e6d7

wbernoudy force-pushed the feature/nary-reduce-node branch from 3a45e03 to f60e6d7 Compare January 10, 2025 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NaryReduceNode and InputNode #166

Add NaryReduceNode and InputNode #166

wbernoudy commented Nov 14, 2024 •

edited

Loading

wbernoudy commented Nov 15, 2024

arcondello left a comment

arcondello Nov 15, 2024

wbernoudy Nov 15, 2024

arcondello Nov 15, 2024

wbernoudy Nov 15, 2024

wbernoudy Nov 15, 2024

arcondello Dec 11, 2024

arcondello Dec 10, 2024

arcondello Dec 11, 2024

arcondello Dec 11, 2024

arcondello Dec 11, 2024

wbernoudy Dec 11, 2024

arcondello Dec 11, 2024

wbernoudy Dec 11, 2024

arcondello Dec 11, 2024

arcondello Dec 11, 2024

arcondello Dec 16, 2024 •

edited

Loading

Add NaryReduceNode and InputNode #166

Are you sure you want to change the base?

Add NaryReduceNode and InputNode #166

Conversation

wbernoudy commented Nov 14, 2024 • edited Loading

wbernoudy commented Nov 15, 2024

arcondello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcondello Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

wbernoudy commented Nov 14, 2024 •

edited

Loading

arcondello Dec 16, 2024 •

edited

Loading