There are two main problems here.
First, there is the algorithmic problem, which is that in every recursive call, you start at the beginning of the list of possible values. That will inevitably lead to duplicated lists. You want the produced lists to be sorted in descending order (or, at least, in the same order as the original list), and you need to maintain that order by not recursing over values you have already finished with.
The second problem is subtler; it has to do with the way you handle the current_combi arrays, with emphasis on the plural. You shouldn't use multiple arrays; when you do, it's very easy to get confused. What you should do is use exactly one array, and only make a copy when you need to add it to the result.
You might need to pull out a pad of paper and a pencil and play computer to see what's going on, but I'll try to describe it. The key is that:
current_combi.append(num) modifies the contents of current_combi;- passing
current_combi as a function argument does not create a new list;
current_combi = current_combi[:-1] does create a new list, and does not modify the old one.
So, you enter getCombinations and create a current_combi list. Then you push the first value onto the new list (so it's now [5] and recursively call getCombinations, passing the same list. Inside the recursive call, the first value is appended again onto the list (which is still the same list); that list is now [5, 5]. That's a valid result, so you add it to the accumulated results, and then you create a new current_combi. At this point, the original current_combi is still [5, 5] and the new one is [5]. Then the for loop continues (in the recursive call), but the rest of that loop no longer has access to the original current_combi. So we can fast forward to the end of the for loop, ignoring the recursive subcalls, and return to the top level.
When we return to the top level, current_combi is the list which was originally created, and that list had 5 appended to it twice, once in the top-level for loop and again when the first recursive call started. So it's still [5. 5], which is unexpected. A fundamental property of recursive backtracking is that the problem state variable be the same before and after each recursive call. But that property has been violated. So now at the end of the top-level for loop, an attempt is made to remove the 5 added at the beginning. But since that list is now [5, 5], removing the last element produces [5] instead of []. As a result, lists starting with 2 are never produced, and lists starting 5, 2 are produced twice.
OK, let's fix that. Instead of making copies of the list at uncontrolled points in the execution, we'll just use one list consistently, and make a copy when we add it to the accumulated results:
# This one still doesn't work. See below.
def getCombinations(num_types, max_amount, current_combi=None):
if current_combi is None:
current_combi = []
for num in num_types:
current_combi.append(num)
if sum(current_combi) == max_amount:
combinations.append(current_combi[:]) # Add a copy to results
elif sum(current_combi) < max_amount:
getCombinations(num_types, max_amount, current_combi)
current_combi.pop() # Restore current_combi
But that doesn't actually fix the first problem noted above: that the recursion should not reuse values which have already been used. Instead of looping over the values in num_types, we need to loop over its suffixes:
def getCombinations(num_types, max_amount, current_combi=None):
if current_combi is None:
current_combi = []
values = num_types[:] # Avoid modifying the caller's list
while values:
# values.pop(0) removes the first element in values and returns it
current_combi.append(values.pop(0))
if sum(current_combi) == max_amount:
combinations.append(current_combi[:]) # Add a copy to results
elif sum(current_combi) < max_amount:
getCombinations(values, max_amount, current_combi)
# Restore current_combi
current_combi.pop()
In the above, I was trying to roughly follow the logic of your original. However, the handling of values could be made more efficient by passing the starting index in the list instead of modifying the list. Also, there is no need to rescan the candidate combination in order to add up its value, since we can just add the value we just added (or, as in the following code, subtract it from the target). Finally, since a lot of the arguments to the recursive call are always the same, they can be replaced by a closure:
def getCombinations(num_types, max_amount):
results = []
candidate = []
def helper(first_index, amount_left):
for index in range(first_index, len(num_types)):
value = num_types[index]
if amount_left == value:
results.append(candidate + [value])
elif amount_left > value:
candidate.append(value)
helper(index, amount_left - value)
candidate.pop()
helper(0, max_amount)
return results
That's still not the optimal implementation, but I hope it shows how to evolve this implementation.
