Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Create new kata: Bounded Knapsack #457

Merged
merged 115 commits into from
Jul 15, 2021

Conversation

louiswmarquis
Copy link
Contributor

I have finished writing the tasks, tests, and reference implementation for the Knapsack Problem kata that I had proposed in #416. I would greatly appreciate if it could be reviewed, and any feedback on how the kata could be improved so that it can be incorporated into the repository. I recognize that the kata for the Knapsack Problem is different and potentially more complex than the other existing Grover katas, so if there are any parts of the new kata that might need clarification, please let me know.

@ghost
Copy link

ghost commented Aug 11, 2020

CLA assistant check
All CLA requirements met.

Copy link
Member

@msoeken msoeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emax-wenjun, thanks a lot for this great contribution. In this review I focussed on the reference implementation. Generally, please fix some tab/space problems consider the use of within/apply for automatic uncomputation. I made several changes which you may batch commit directly in Github, and for some just referenced the corresponding keyword.

BoundedKnapsack/BoundedKnapsack.csproj Outdated Show resolved Hide resolved
BoundedKnapsack/BoundedKnapsack.csproj Outdated Show resolved Hide resolved
BoundedKnapsack/Hints.qs Outdated Show resolved Hide resolved
BoundedKnapsack/Hints.qs Outdated Show resolved Hide resolved
BoundedKnapsack/Hints.qs Outdated Show resolved Hide resolved
BoundedKnapsack/ReferenceImplementation.qs Outdated Show resolved Hide resolved
BoundedKnapsack/ReferenceImplementation.qs Outdated Show resolved Hide resolved
BoundedKnapsack/ReferenceImplementation.qs Outdated Show resolved Hide resolved
BoundedKnapsack/ReferenceImplementation.qs Outdated Show resolved Hide resolved
BoundedKnapsack/ReferenceImplementation.qs Outdated Show resolved Hide resolved
louiswmarquis and others added 17 commits August 11, 2020 21:20
Co-authored-by: Mathias Soeken <[email protected]>
Co-authored-by: Mathias Soeken <[email protected]>
Co-authored-by: Mathias Soeken <[email protected]>
Co-authored-by: Mathias Soeken <[email protected]>
Co-authored-by: Mathias Soeken <[email protected]>
Co-authored-by: Mathias Soeken <[email protected]>
Co-authored-by: Mathias Soeken <[email protected]>
Co-authored-by: Mathias Soeken <[email protected]>
Co-authored-by: Mathias Soeken <[email protected]>
@louiswmarquis
Copy link
Contributor Author

louiswmarquis commented Oct 19, 2020

@tcNickolas Hi. Have you had a chance to take a look at the rest of the kata?

@louiswmarquis
Copy link
Contributor Author

Hi @tcNickolas, have you had a chance to review the remainder of the pull request? Thanks.

Copy link
Member

@tcNickolas tcNickolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, I'm really sorry it took me so long to get back to this review! It is a topic that requires some serious focus to solve through, definitely one of the hardest katas we have to date! And this week was the first one I had in over half a year which allowed me to spend several days solving through the kata.

I reviewed and edited the first two chapters of the kata. There are a lot of edits, a lot of them are focused on code style, using descriptive variable names and unifying the approach we're using for testing. There were a few more interesting edits as well:

  • There was a subtle bug in test generation in part II - generating item counts without taking into account item limits caused the test fail on a correct solution in several tasks when the solution received item counts over the limit. I modified it to generate only valid data.
  • I unified testing harnesses for tasks 1.4 and 1.5, 1.6 and 1.7, and 2.7 and 2.8 - the tests and the tasks for these pairs followed exactly the same structure, and the differences could be isolated in the operation that was being tested and the comparator used in the last check. I suspect there might be more space for test unification here - most of the tests look very similar - but I don't have a good idea for how to do it at the moment.
  • I modified task 2.2 to do the required transformation for a generic array, rather than an array of qubits - it made it easier to make sure the transformation is correct, and made the code more reusable in other places.

Part III of the kata requires more work:

  1. I think it makes a lot of sense to add two more tasks to solve 01 version of the knapsack problem before jumping into the bounded knapsack version, to let the learner start with a simpler version
  2. I'm very reluctant to use classical solution counting even as a placeholder, since that involves iterating over all possible inputs and processing them, and that makes using Grover's search pretty much pointless. I think the best thing here is to use an approach similar to SolveSATWithGrover, which just tries search with different numbers of iteration.
  3. I'd also love to look into adding more test cases to the tasks in part III - right now they're only solving 1 test, and it takes 45 seconds, so adding test cases requires looking into the efficiency of the solution - might require reducing the size of the test cases etc.

For now I removed part III from the kata altogether. I want to merge this PR, so as to unblock further work on it that can be done in parallel (developing the Jupyter Notebook "frontend" for the tasks, developing the workbook, and finishing part III) - and to safeguard against me disappearing for another couple of months! I'll file the issues for this work separately.

Do you by any chance have a link to the paper you wrote about this algorithm, on arxiv or elsewhere? It could be very useful for the learner to use it together with the kata until we can develop the workbook for it.

Thank you so much for contributing this! This is a massive effort, and it will make a great kata!

@tcNickolas tcNickolas merged commit 5b32de9 into microsoft:main Jul 15, 2021
@louiswmarquis
Copy link
Contributor Author

@tcNickolas Thank you so much for thoroughly reviewing and editing this kata! I've only just had the opportunity to read the changes, and I can probably resume working on further changes in the near future. As for the paper, I don't think I'm allowed to give it out prodigally since it's published in a journal, but I might be able to get you a copy if you privately message me. I'm excited to continue working on this kata!

@tcNickolas
Copy link
Member

@emax-wenjun It will be great if you have the time to finish the kata! I filed issue #638 with some extra thoughts on the last chapter but haven't gotten to doing it yet, so if you want to pick it up, great! I'll try to be more responsive with the next review, I promise!

@tcNickolas
Copy link
Member

@emax-wenjun I wonder if you'd be interested in writing a blog post about this algorithm and its Q# implementation for Q# Advent Calendar? You could probably use some of the text from your paper, and it would be a great learning resource to accompany the kata (as well as a nice way to advertise it!)
Let me know if that sounds interesting!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quantum Oracle for Solving (Bounded) Knapsack Problem
3 participants