fixing knapsack_value_weight()

[AntonSimen06]

the function parameters are lists and cannot be multiplied directly. The function has been corrected so that multipication is done between each element of the lists so that the total weight and the total value are obtained.

Summary

Details and comments

[AntonSimen06]

a remark: the function get_solution() returns a list.

[woodsp-ibm]

This test case https://github.com/Qiskit/qiskit-aqua/blob/main/test/optimization/test_knapsack.py does go through the function you have changed and shows it works as it exists now. The 'solution' is a numpy array, the knapsack_weight_value says that the arguments should be all of type numpy array. You will find that the multiplication on numpy arrays is elementwise and so we do get the right answer - but yes it does not work with plain lists - weights and values though in the test case are lists, but since solution is a numpy array it still goes through numpy.multiply (via the * operator). Do you have some case where it did not work?

[AntonSimen06]

This test case https://github.com/Qiskit/qiskit-aqua/blob/main/test/optimization/test_knapsack.py does go through the function you have changed and shows it works as it exists now. The 'solution' is a numpy array, the knapsack_weight_value says that the arguments should be all of type numpy array. You will find that the multiplication on numpy arrays is elementwise and so we do get the right answer - but yes it does not work with plain lists - weights and values though in the test case are lists, but since solution is a numpy array it still goes through numpy.multiply (via the * operator). Do you have some case where it did not work?

Hi Wood. I did the procedure as follow:

I used a very simple example so that weights=[2,1] and values=[200,50] and max_weight=200 (we know the solution [1,0]). ok

1 - print coefficients and pauli_op from get_operator(); 2 - construct VQE from scratch and get the minimum eigenvalue of pauli_op and the "ground state" which also is a list of zeros and ones (in this case I found [1,0,0,1]); 3 - use the function solution=get_solution() which give me solution=[1,0] 4 - use knapsack_value_weight() to find the value and weight inside knapsack (200 , 2).

In step 4 I faced with this error: TypeError: can't multiply sequence by non-int of type 'list'. In order to solve this I did a test changing the source of knapsack_value_weight() as I've proposed.

I also could do:

value = np.sum(np.multiply(solution*values))
weight=np.sum(np.multiply(solution*weights))

Does something wrong with I am trying to do?

[woodsp-ibm]

The only thing you are really doing "wrong" is that both get_solution and knapsack_value_weight state they take numpy arrays as types not plain python lists. Eg https://github.com/Qiskit/qiskit-aqua/blob/aa8bc0c8011405d382fc05d6ab576dd926bdd2a7/qiskit/optimization/applications/ising/knapsack.py#L200-L207

Now the way knapsack_weight_value works it happens that as long as solution is a numpy array then weights and values can be plain lists, as you saw in the test case I referenced. And seems that if x is a plain list in get solution that method ends up still working but returns a plain list which cannot then be given to knapsack_weight_value. To "fix" things in your case if you simply did np.asarray(x) to whatever the x value is you pass to get_solution() then I think it should all work for you.

Now I agree it might be an improvement if it all worked with plain lists like you are using. I will note, since the last release at the start of April 2021, Aqua is deprecated, see https://github.com/Qiskit/qiskit-aqua#qiskit-aqua-now-deprecated, for more information. The optimization code has been moved to its own repository and the knapsack code, and other ising converters from here, now produce QuadraticPrograms that can be used more generally with the optimization function. See here https://github.com/Qiskit/qiskit-optimization/blob/main/qiskit_optimization/applications/knapsack.py and here is a test case using this new form https://github.com/Qiskit/qiskit-optimization/blob/main/test/applications/test_knapsack.py While Aqua is deprecated, it is still being supported, but changes are being kept to any critical bug fixes and to maintaining compatibility with the other parts of Qiskit in this period. Since what is there works I think, as long as you use the types that it says in the docstrings of the functions, while I agree it might be nice if it worked directly with lists that's not really a critical bug fix.

Does that explanation help, and I hope you understand.

[AntonSimen06]

The only thing you are really doing "wrong" is that both get_solution and knapsack_value_weight state they take numpy arrays as types not plain python lists. Eg

https://github.com/Qiskit/qiskit-aqua/blob/aa8bc0c8011405d382fc05d6ab576dd926bdd2a7/qiskit/optimization/applications/ising/knapsack.py#L200-L207

Now the way knapsack_weight_value works it happens that as long as solution is a numpy array then weights and values can be plain lists, as you saw in the test case I referenced. And seems that if x is a plain list in get solution that method ends up still working but returns a plain list which cannot then be given to knapsack_weight_value. To "fix" things in your case if you simply did np.asarray(x) to whatever the x value is you pass to get_solution() then I think it should all work for you.

Now I agree it might be an improvement if it all worked with plain lists like you are using. I will note, since the last release at the start of April 2021, Aqua is deprecated, see https://github.com/Qiskit/qiskit-aqua#qiskit-aqua-now-deprecated, for more information. The optimization code has been moved to its own repository and the knapsack code, and other ising converters from here, now produce QuadraticPrograms that can be used more generally with the optimization function. See here https://github.com/Qiskit/qiskit-optimization/blob/main/qiskit_optimization/applications/knapsack.py and here is a test case using this new form https://github.com/Qiskit/qiskit-optimization/blob/main/test/applications/test_knapsack.py While Aqua is deprecated, it is still being supported, but changes are being kept to any critical bug fixes and to maintaining compatibility with the other parts of Qiskit in this period. Since what is there works I think, as long as you use the types that it says in the docstrings of the functions, while I agree it might be nice if it worked directly with lists that's not really a critical bug fix.

Does that explanation help, and I hope you understand.

Thank you for your answer. Yes, it will help me. From now on I'll do as you have mentioned.

[woodsp-ibm]

You are welcome. You might also like to take a look at qiskit-optimization which is where all the optimization code from here was moved to and where all the work is being done now. The aqua repository here and its code are no longer being developed - I mentioned the deprecation above - so its advisable to migrate to the new code at your earliest convenience.

[AntonSimen06]

You are welcome. You might also like to take a look at qiskit-optimization which is where all the optimization code from here was moved to and where all the work is being done now. The aqua repository here and its code are no longer being developed - I mentioned the deprecation above - so its advisable to migrate to the new code at your earliest convenience.

Great! I'll follow your advice and migrate to new code. Thanks

[woodsp-ibm]

You'll find things a little different for knapsack etc, as I mentioned above. Closing this now then as resolved.