Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate nonlocal games 1596 task1 - quantum #1745

Merged
merged 19 commits into from
Aug 1, 2024

Conversation

ggridin
Copy link
Contributor

@ggridin ggridin commented Jul 16, 2024

The link to the issue: #1596

This pull request is covering

  1. CHSH quantum tasks (2.2, 2.4; 2.5 is converted to a demo)
  2. CHSH discussion
  3. Few changes in CHSH classical

@Manvi-Agrawal
Copy link
Contributor

Manvi-Agrawal commented Jul 17, 2024

chsh_quantum_bob_strategy (task 2.2) is added. please note that base profile has error "cannot use dynamic bool value".- is there fix or workaround for this problem?

  • I think we would be fine with that, since katas use unrestricted profile.

Quantinuum emulator profile has error "cannot use dynamic bool/string value".

@swernli
Copy link
Collaborator

swernli commented Jul 24, 2024

chsh_quantum_bob_strategy (task 2.2) is added. please note that base profile has error "cannot use dynamic bool value".- is there fix or workaround for this problem?

  • I think we would be fine with that, since katas use unrestricted profile.

Quantinuum emulator profile has error "cannot use dynamic bool/string value".

Yes, that's expected. The katas exercises run Unrestricted and do not need to be compatible with QIR Base profile.

Copy link
Contributor

@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.

Apologies for the delay reviewing this PR, I had a very busy week :-(
Here are the comments for most of the changes, except for the ones in the root index.md - I'll review that file a bit later today.

Thank you!

Copy link
Contributor

@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.

Finished reading through the index.md, posting the rest of the comments.

Additionally, could you please edit the PR description to move out everything starting with "other PRs will follow"? These details are good to include for the reviewer while the PR is in progress, but our merge system is set up in a way that includes the whole PR description in the commit message, and they're not necessary there, just making the commit info look bulky. For future PRs, these details can be made into the first comment on the PR rather than a part of the description.

Thank you!!

@ggridin
Copy link
Contributor Author

ggridin commented Jul 29, 2024

Additional details of this PR:

  1. Section "CHSH Classical" is merged with "CHSH Game"
    • DrawRandomBool is used instead of DrawRandomInt
  2. chsh_quantum_alice_strategy (task 2.2) is added
  3. chsh_quantum_bob_strategy (task 2.2) is added
    • please note that base profile has error "cannot use dynamic bool value".- is there fix or workaround for this problem?
  4. Discussion content is copied from previous kata version
  5. Task 2.5 is significantly refactored - needs thoughtful review.
    The benefits of the change:
    • Demo is comparison of classic and quantum results - which is interesting to observe.
    • Entangled qubits are created before the game.
  6. Few cosmetic changes in nonlocal_games/index.md suggested by vscode markdown linter

Testing done:

  1. Build "python ./build.py --npm" is successful
  2. Placeholder cases are syntactically correct but failing verifications.
    Solutions are successful.
  3. Some other failure scenarios were tested manually.
  4. Content screenshots are attached.
  5. Demo is tested in Azure Quantum coding (in-memory simulator): https://quantum.microsoft.com/en-us/experience/quantum-coding
    • Quantinuum emulator profile has error "cannot use dynamic bool/string value".

CHSH Alice Quantum Strategy
CHSH Base profile error
CHSH Bob Quantum Strategy
CHSH Classic Lesson title is removed
CHSH Discussion Case 1
CHSH Discussion Case 2
CHSH Running CHSH game - results
CHSH Running CHSH game - text

@ggridin ggridin requested a review from tcNickolas July 29, 2024 22:33
@ggridin
Copy link
Contributor Author

ggridin commented Jul 29, 2024

Finished reading through the index.md, posting the rest of the comments.

Additionally, could you please edit the PR description to move out everything starting with "other PRs will follow"? These details are good to include for the reviewer while the PR is in progress, but our merge system is set up in a way that includes the whole PR description in the commit message, and they're not necessary there, just making the commit info look bulky. For future PRs, these details can be made into the first comment on the PR rather than a part of the description.

Thank you!!

@tcNickolas
Thank you for the detailed and thoughtful review!
I believe that I fixed all mentioned issues. I hope I wont miss something, considering amount of changes.

In the current PR, I've updated PR description according your suggestion and moved the details to one of the comments.
I'll do the same shortly for #1783 too

Copy link
Contributor

@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.

Looks great! And the updated demo is much more illustrative of the differences between classical and quantum strategies!

I left a few minor comments, but I shouldn't need to take another look. Just need a second reviewer's signoff.

Thank you!

@tcNickolas tcNickolas added this pull request to the merge queue Aug 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 1, 2024
@tcNickolas tcNickolas added this pull request to the merge queue Aug 1, 2024
Merged via the queue into microsoft:main with commit 2e6ffa1 Aug 1, 2024
19 checks passed
@ggridin ggridin deleted the MigrateNonlocalGames-1596-task1 branch August 9, 2024 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants