Enhancement/axis wise bounds NumberNode#471
Enhancement/axis wise bounds NumberNode#471fastbodin wants to merge 25 commits intodwavesystems:mainfrom
Conversation
fastbodin
force-pushed
the
enhancement/axis_wise_bounds_numbernode
branch
2 times, most recently
from
bebd145 to
15f7b67
Compare
|
Feature is fully implemented. |
fastbodin
force-pushed
the
enhancement/axis_wise_bounds_numbernode
branch
from
e3b0255 to
1c177b4
Compare
|
Closes #216. |
| /// constraints on the sum of the values in each slice along `axis`. | ||
| /// Constraints can be defined for ALL slices along `axis` or PER slice along | ||
| /// `axis`. Allowable operators are defined by `BoundAxisOperator`. | ||
| struct BoundAxisInfo { |
There was a problem hiding this comment.
Aesthetic, but I'd suggest calling this AxisBound and nesting BoundAxisOperator in it and calling that Operator. Also enum class. So NumberNode::AxisBound::Operator::Equal. A bit more succinct and IMO more clearly organized.
There was a problem hiding this comment.
We could also keep using an 🤷enum, so NumberNode::AxisBound::Equal but I usually prefer enum classes
Or keep the current.
Nah, changed my mind. enum class. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#renum-class
There was a problem hiding this comment.
Agreed on all fronts. Much cleaner.
dwave/optimization/include/dwave-optimization/nodes/numbers.hpp
Outdated
Show resolved
Hide resolved
dwave/optimization/include/dwave-optimization/nodes/numbers.hpp
Outdated
Show resolved
Hide resolved
|
First round of comments addressed. |
fastbodin
force-pushed
the
enhancement/axis_wise_bounds_numbernode
branch
from
4b2205c to
c6a93d5
Compare
|
I do see "hyperslice" used in a lot of comments. Would prefer that is changed to just "slice" as well, or at least a small comment maybe in the header |
Data is stored at C++ level with the class `AxisBoundInfo` as private attribute to `NumberNode`. Added relevant C++ tests.
For each bound axis and each hyperslice along said axis, we store the running sum of the values within the hyperslice. This state dependant data is stored via `NumberNodeStateData`. If `NumberNode` is initialized with values, we check that all axis-wise bounds are satisfied.
Added satisfies_axis_wise_bounds(), update_bound_axis_slice_sums(), axis_wise_bounds(), and bound_axis_sums() to NumberNode. Updated various NumberNode, IntegerNode, and BinaryNode methods to reference NumberNodeStateData as opposed to ArrayNodeStateData. Updated all NumberNode mutate methods to reflect changes to the axis-wise bound running sums. Added C++ tests to check said mutate methods on BinaryNode and IntegerNode.
Make use of BufferIterators to compute the sum of the values within each hyperslice along each bound axis as opposed making a custom method to do this.
Defined method to initialize_state() given exactly one axis-wise bound. Fill state with lower bounds and increment until state satisfies axis-wise bounds or determines infeasible. Added appropriate C++ IntegerNode and BinaryNode tests.
Made BoundAxisInfo and BoundAxisOperators members of NumberNode. Updated all C++ tests. Optimized BufferIterator use in initialize_state().
Improved comments. Improved methods. Cleaned up C++ tests. Added static_casts where necessary for CircleCI.
When constructing a state that satisfies exactly one axis-wise bound.
Needed for Cython interface. Changed relevant C++ code. Used aliases in C++ NumberNode tests.
Also made BoundAxisOperaters an `enum` as opposed to `enum class` because we found a Cython workaround.
Resolved conflict in rebase.
Previously accepted list of tuples and list of lists. Now simply list of tuples for consistency.
Specific to axis-wise bounds.
`BoundAxisInfo` -> `AxisBound` and `BoundAxisOperator` -> `Operator`. `Operator` is now a nested enum classs of `AxisBound`.
Added indicator variable that all bound axis operators are `==` to reduce redundancy in `NumberNode::exchange()` method.
fastbodin
force-pushed
the
enhancement/axis_wise_bounds_numbernode
branch
from
c6a93d5 to
72c6d96
Compare
|
Second round of comments addressed. |
arcondello
left a comment
arcondello
left a comment
There was a problem hiding this comment.
Looks great! Just two aesthetic comments
| AxisBound(ssize_t axis, std::vector<Operator> axis_operators, | ||
| std::vector<double> axis_bounds); | ||
|
|
||
| /// The bound axis |
There was a problem hiding this comment.
It seems a bit inconsistent to have both public members and getter functions... I do see that the getters provide some functionality, so there might be instances where you'd like the choice. But I'm tempted to say you should make these private and force everyone to use the getter functions.
There was a problem hiding this comment.
I think the honest answer is I made them public for ease of C++ testing. Agreed is inconsistent. Will change 👍.
| if (bound_axis_sums[slice] < bound_axis_info.get_bound(slice)) return false; | ||
| break; | ||
| default: | ||
| unreachable(); |
There was a problem hiding this comment.
I do like putting an assert(false && "unexpected operator type") here for later debugging.
Made axis, operators, and bounds private members. Added axis(), num_bounds(), and num_operators() methods. Updated C++ code/tests, Python, and Cython to reflect this.
Removed `asserts()` that axis-wise bounds were satisfied to `NumberNode::propagate()`.
|
Third round of comments addressed. |
3 participants
Do not merge. Draft PR.
Only C++ has been implemented.Simply checking CircleCi.