-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Create new kata: Bounded Knapsack #457
Create new kata: Bounded Knapsack #457
Conversation
There was a problem hiding this 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.
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]>
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]>
@tcNickolas Hi. Have you had a chance to take a look at the rest of the kata? |
Hi @tcNickolas, have you had a chance to review the remainder of the pull request? Thanks. |
* Document several helper functions * Update to QDK 0.18
There was a problem hiding this 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:
- 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
- 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.
- 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 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! |
@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! |
@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!) |
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.