Add dynamic reduce sum propagator#504
Add dynamic reduce sum propagator#504alexzucca90 wants to merge 10 commits intodwavesystems:constraint-programmingfrom
Conversation
|
|
||
| namespace dwave::optimization::cp { | ||
|
|
||
| // enum class CPConversionSatus { OK, Failed }; |
There was a problem hiding this comment.
Did you mean to leave this commented out code in this PR?
There was a problem hiding this comment.
I think I want to remove this file now :)
| std::vector<double> min, max; | ||
| }; | ||
|
|
||
| // ----- ReducePropagator ----- |
There was a problem hiding this comment.
That should the correct one, below you can find the ReducePropagator methods implementation
| while (data->num_indices_to_process() > 0) { | ||
| ssize_t i = indices_to_process.front(); | ||
| indices_to_process.pop_front(); | ||
| data->set_scheduled(false, i); |
There was a problem hiding this comment.
I think this needs to be at the end of loop to prevent the index from getting scheduled again?
There was a problem hiding this comment.
I think it should happen there. Say I prune an adjacent variable, then modifying that variable could trigger another propagation. If I unschedule this index, then I'm preventing this extra propagation.
There is a caveat in all this. Can we guarantee that all the pruning that is done here, won't need another propagation?
| p_state[propagator_index_] = std::make_unique<PropagatorData>(state.get_state_manager(), 1); | ||
| } | ||
|
|
||
| /// Compute the upper and lower bounds derived from the definitely present variables |
There was a problem hiding this comment.
Probably worth noting for now that this is only for the + operator
| ssize_t i = indices_to_process.front(); | ||
| indices_to_process.pop_front(); | ||
| data->set_scheduled(false, i); | ||
| assert(i == 0); |
There was a problem hiding this comment.
If this code is meant to only ever work for full reduction, so output size is always scalar, I would delete the while loop.
If you want to make it work for partial reduction later, I would add a comment about that at least to this assert
|
|
||
| for (ssize_t j = 0; j < static_cast<ssize_t>(lb_optional.size()); ++j) { | ||
| // Update the min size if the domains overlap | ||
| if (not(ub_optional[j] < out_min or out_max < lb_optional[j])) { |
There was a problem hiding this comment.
| if (not(ub_optional[j] < out_min or out_max < lb_optional[j])) { | |
| if (not (ub_optional[j] < out_min or out_max < lb_optional[j])) { |
| } | ||
|
|
||
| assert(min_size_new >= in_->min_size(v_state)); | ||
| bool changed_min_size = (min_size_new > in_->min_size(v_state)) ? true : false; |
There was a problem hiding this comment.
| bool changed_min_size = (min_size_new > in_->min_size(v_state)) ? true : false; | |
| bool changed_min_size = min_size_new > in_->min_size(v_state); |
| // first compute the widest possible sum that there can be gven present and optional | ||
| // variables | ||
| { | ||
| double sum_max = *std::max_element(ub_optional.begin(), ub_optional.end()); |
There was a problem hiding this comment.
Can this fail if changed_min_size == in_->max_size(state)?
| assert(not in_->is_active(v_state, in_->min_size(v_state) + j)); | ||
| assert(in_->maybe_active(v_state, in_->min_size(v_state) + j)); | ||
|
|
||
| auto smin_it = std::min_element(lb_optional.begin() + j + 1, lb_optional.end()); |
There was a problem hiding this comment.
Might be worth pre-computing these outside the loop to avoid the O((max_size - min_size)^2) cost here?
| dwave::optimization::Graph graph; | ||
|
|
||
| // Add an integer node | ||
| ListNode* x = graph.emplace_node<dwave::optimization::ListNode>(5, 0, 5); |
There was a problem hiding this comment.
Possibly worth another test case that allows negative values to ensure the minimums are working properly as well...
2 participants
Add a bound consistent sum propagator that handles dynamic input array