Feature/partial sum #114

alexzucca90 · 2024-09-20T17:08:02Z

Added partial sum node that allows to do stuff like

from dwave.optimization import Model
model = Model()
x = model.integer((3, 3))
y = x.sum(axis=0)
z = x.sum(axis=1)

alexzucca90 · 2024-09-20T17:10:00Z

Interesting, tests are fine on my side.. checking again.

alexzucca90 · 2024-09-20T21:38:02Z

Just realized, this (partially) closes #47

dwave/optimization/model.pyx

releasenotes/notes/partial-sum-node-ca3356c3bddfacde.yaml

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

dwave/optimization/src/nodes/mathematical.cpp

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

dwave/optimization/symbols.pyx

dwave/optimization/symbols.pyi

dwave/optimization/src/array.cpp

dwave/optimization/src/nodes/mathematical.cpp

tests/cpp/tests/test_array.cpp

arcondello · 2024-09-25T15:57:31Z

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

+                                          const ssize_t axis);
+
+/// Gets the strides of a n-dimensional array assuming contiguous memory
+std::vector<ssize_t> as_contiguous_strides(const std::span<const ssize_t> shape);


This duplicates the functionality of the existing shape_to_strides() static method. Though of course it has a slightly different signature.

I moved this behind compilation barrier in mathematical.cpp.

alexzucca90 · 2024-09-26T22:40:11Z

Ok I tried to make the code ready to support multiple axes in the future. I pushed single axis assumption behind the compilation barrier. Can you check the serialization/deserialization?

arcondello · 2024-10-02T17:55:29Z

dwave/optimization/model.pyi

@@ -178,4 +178,4 @@ class ArraySymbol(Symbol):
    def state(self, index: int = 0, *, copy: bool = True) -> numpy.ndarray: ...
    def state_size(self) -> int: ...
    def strides(self) -> typing.Tuple[int, ...]: ...
-    def sum(self) -> Sum: ...
+    def sum(self, axis:typing.Optional[int] = None) -> typing.Union[Sum, PartialSum]: ...


Suggested change

def sum(self, axis:typing.Optional[int] = None) -> typing.Union[Sum, PartialSum]: ...

def sum(self, axis: typing.Optional[int] = None) -> typing.Union[Sum, PartialSum]: ...

arcondello · 2024-10-02T17:57:24Z

dwave/optimization/model.pyx

+        from dwave.optimization.symbols import PartialSum # avoid circular import
        from dwave.optimization.symbols import Sum  # avoid circular import
+
+        if axis is not None:
+            if not isinstance(axis, numbers.Integral):
+                raise TypeError("axis of the sum should be an int")
+
+            if not (0 <= axis < self.ndim()):
+                raise ValueError("axis should be 0 <= axis < self.ndim()")
+
+            return PartialSum(self, axis)
+
        return Sum(self)


Suggested change

from dwave.optimization.symbols import PartialSum # avoid circular import

from dwave.optimization.symbols import Sum # avoid circular import

if axis is not None:

if not isinstance(axis, numbers.Integral):

raise TypeError("axis of the sum should be an int")

if not (0 <= axis < self.ndim()):

raise ValueError("axis should be 0 <= axis < self.ndim()")

return PartialSum(self, axis)

return Sum(self)

if axis is not None:

if not isinstance(axis, numbers.Integral):

raise TypeError("axis of the sum should be an int")

if not (0 <= axis < self.ndim()):

raise ValueError("axis should be 0 <= axis < self.ndim()")

return dwave.optimization.symbols.PartialSum(self, axis)

return dwave.optimization.symbols.Sum(self)

This pattern works too and doesn't make an unnecessary import.

this does not compile, doesn't know what dwave is of course..

Ah, right. You can add import dwave.optimization.symbols back at the top of the function.

but then we're importing more, aren't we?

This pattern works too and doesn't make an unnecessary import.

the unnecessary bit was adding two symbols to the namespace. The import happens either way.

This is all very much aesthetic stuff. I doubt the performance difference is even noticeable.

arcondello · 2024-10-02T17:58:41Z

dwave/optimization/src/array.cpp

+    ssize_t ndim = static_cast<ssize_t>(strides.size());
+    std::vector<ssize_t> indices;
+    indices.reserve(ndim);


Suggested change

ssize_t ndim = static_cast<ssize_t>(strides.size());

std::vector<ssize_t> indices;

indices.reserve(ndim);

std::vector<ssize_t> indices;

indices.reserve(strides.size());

looks like this is the only place ndim is used. And this way we avoid casting to ssize_t and then back to size_t.

arcondello · 2024-10-02T18:02:34Z

dwave/optimization/src/array.cpp

+    std::vector<ssize_t> indices;
+    indices.reserve(ndim);
+
+    for (auto stride : strides) {


Suggested change

for (auto stride : strides) {

for (const auto& stride : strides) {

arcondello · 2024-10-02T18:03:05Z

dwave/optimization/src/array.cpp

+
+ssize_t ravel_multi_index(const std::span<const ssize_t> strides,
+                          const std::span<const ssize_t> indices) {
+    ssize_t ndim = static_cast<ssize_t>(strides.size());


Suggested change

ssize_t ndim = static_cast<ssize_t>(strides.size());

const ssize_t ndim = static_cast<ssize_t>(strides.size());

arcondello · 2024-10-02T18:07:20Z

dwave/optimization/src/nodes/mathematical.cpp

+    std::vector<ssize_t> shape;
+    shape.assign(input_shape.begin(), input_shape.end());


Suggested change

std::vector<ssize_t> shape;

shape.assign(input_shape.begin(), input_shape.end());

std::vector<ssize_t> shape(input_shape.begin(), input_shape.end());

arcondello · 2024-10-02T18:08:18Z

dwave/optimization/src/nodes/mathematical.cpp

+std::vector<ssize_t> as_contiguous_strides(const std::span<const ssize_t> shape) {
+    ssize_t ndim = static_cast<ssize_t>(shape.size());
+
+    assert(ndim >= 0);


assert(ndim > 0);

right? because we access strides[ndim - 1] shortly.

Or, if we want to handle the ndim == 0 case, we could do

assert(ndim >= 0); if (ndim < 1) return {};

arcondello · 2024-10-02T18:11:10Z

dwave/optimization/src/nodes/mathematical.cpp

+template <class BinaryOp>
+PartialReduceNode<BinaryOp>::PartialReduceNode(ArrayNode* node_ptr, std::span<const ssize_t> axes,
+                                               double init)
+        : ArrayOutputMixin(partial_reduce_shape(node_ptr->shape(), axes[0])),


This will segfault with axes.size() < 1.

Unfortunately std::span::at() doesn't come in until C++26, otherwise that would be an elegant fix.

I think you'll need to add a

std::span<const ssize_t> nonempty(std::span<const ssize_t> span) { if (span.empty()) throw ...; return span; }

function and then use it here

: ArrayOutputMixin(partial_reduce_shape(node_ptr->shape(), nonempty(axes)[0])),

dwave/optimization/src/nodes/mathematical.cpp

arcondello · 2024-10-02T18:21:32Z

dwave/optimization/src/nodes/mathematical.cpp

+        : PartialReduceNode(array_ptr, axis, 0) {}
+
+template <class BinaryOp>
+PartialReduceNode<BinaryOp>::PartialReduceNode(ArrayNode* array_ptr, std::span<const ssize_t> axes)


I don't think we want this generic overload, right? It doesn't set up init and we already cover the one case that does.

so remove that also from the header?

So the header file keeps it. But we only implement it for the relevant type overloads.

arcondello

LGTM

arcondello · 2024-10-02T21:44:24Z

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

@@ -23,6 +23,7 @@
 #include <memory>
 #include <numeric>
 #include <optional>
+#include <ranges>


Do we use ranges anywhere here?

I used to, not now removing it

alexzucca90 force-pushed the feature/partial_sum branch from e1f1fb2 to da57e28 Compare September 20, 2024 19:45

alexzucca90 added the enhancement New feature or request label Sep 20, 2024

alexzucca90 force-pushed the feature/partial_sum branch from da57e28 to cbc070b Compare September 24, 2024 13:26

arcondello reviewed Sep 24, 2024

View reviewed changes

alexzucca90 commented Sep 25, 2024

View reviewed changes

dwave/optimization/symbols.pyi Show resolved Hide resolved

alexzucca90 force-pushed the feature/partial_sum branch from 3aa9414 to 7bee0fd Compare September 25, 2024 03:25

arcondello reviewed Sep 25, 2024

View reviewed changes

alexzucca90 force-pushed the feature/partial_sum branch from 854653e to 4c484f3 Compare September 27, 2024 13:48

arcondello reviewed Oct 2, 2024

View reviewed changes

alexzucca90 force-pushed the feature/partial_sum branch from 4c484f3 to 2867aa1 Compare October 2, 2024 21:11

arcondello approved these changes Oct 2, 2024

View reviewed changes

azucca added 15 commits October 2, 2024 16:15

Added partial sum node

1870899

Added tests for partial sum

d0590e1

Add release notes

d813633

Fix missing resize in initial values

33a615f

Renamed cpp tests for consistency

4326b56

Moved tests in alphabetical order

b387c32

NFC, cosmetic changes in sum method

3effbd2

NFC, cosmetic changes in sum method

178b299

NFC, cosmetic change in error message

5a66cc1

Added missing line

d30e02f

Add missing parenthesis

ae36f5f

Avoid recalculating end of loop at every iteration

745bffe

Corrected integral() method

c80b77a

Cosmetic change for clarity

973be2f

Fixed typo

670b979

azucca added 14 commits October 2, 2024 16:15

Reverted formatting changes

90b5eba

Sorted methods alphabetically

f6b2b3a

Renamed method and sorted alphabetically

12cc112

Updated min max methods

302c1ad

Removed unused extra array data

02863aa

Reworked partial reduction population

1c04918

NFC, bumped item number of description

81aa26e

Future proof for multiple axes

a44c93c

Moved some functions behind compilation barrier

00e32d4

Missing space

858aaa3

Reducing imports in .sum()

27a3707

Fixed sizes asserts and const

30e1b01

Various fixes

c6b0722

Remove ranges dependency

a9544ab

alexzucca90 force-pushed the feature/partial_sum branch from 9d42944 to a9544ab Compare October 2, 2024 23:16

arcondello linked an issue Oct 2, 2024 that may be closed by this pull request

Consider allowing reduction along a single axis #47

Closed

arcondello merged commit 2afeb10 into dwavesystems:main Oct 2, 2024
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/partial sum #114

Feature/partial sum #114

alexzucca90 commented Sep 20, 2024

alexzucca90 commented Sep 20, 2024

alexzucca90 commented Sep 20, 2024

arcondello Sep 25, 2024 •

edited

Loading

alexzucca90 Sep 26, 2024

alexzucca90 commented Sep 26, 2024

arcondello Oct 2, 2024

arcondello Oct 2, 2024

alexzucca90 Oct 2, 2024

arcondello Oct 2, 2024

alexzucca90 Oct 2, 2024

arcondello Oct 2, 2024

arcondello Oct 2, 2024

arcondello Oct 2, 2024

arcondello Oct 2, 2024

arcondello Oct 2, 2024

arcondello Oct 2, 2024

arcondello Oct 2, 2024

arcondello Oct 2, 2024

alexzucca90 Oct 2, 2024

arcondello Oct 2, 2024

arcondello left a comment

arcondello Oct 2, 2024

alexzucca90 Oct 2, 2024

	def sum(self, axis:typing.Optional[int] = None) -> typing.Union[Sum, PartialSum]: ...
	def sum(self, axis: typing.Optional[int] = None) -> typing.Union[Sum, PartialSum]: ...

	for (auto stride : strides) {
	for (const auto& stride : strides) {

	ssize_t ndim = static_cast<ssize_t>(strides.size());
	const ssize_t ndim = static_cast<ssize_t>(strides.size());

		std::vector<ssize_t> shape;
		shape.assign(input_shape.begin(), input_shape.end());

	std::vector<ssize_t> shape;
	shape.assign(input_shape.begin(), input_shape.end());
	std::vector<ssize_t> shape(input_shape.begin(), input_shape.end());

Feature/partial sum #114

Feature/partial sum #114

Conversation

alexzucca90 commented Sep 20, 2024

alexzucca90 commented Sep 20, 2024

alexzucca90 commented Sep 20, 2024

arcondello Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzucca90 commented Sep 26, 2024

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcondello Sep 25, 2024 •

edited

Loading