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

Optimizations to reduce qubits based on non-entangled squares #210

Merged
merged 102 commits into from Jan 13, 2022

Conversation

madcpf
Copy link
Collaborator

@madcpf madcpf commented Sep 5, 2021

Reduce number of qubits used in the following cases:

  • SPLIT_SLIDE and MERGE_SLIDE: if an arm of the slide/merge only has one path qubit then that qubit could be used directly as the path qubit, instead of adding a new ancilla qubit;
  • CAPTURE: don't create a capture ancilla if the attacker is known to be there and there is only one path qubit.

Copy link
Collaborator

@losos0 losos0 left a comment

Choose a reason for hiding this comment

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

Everything looks great!

@madcpf
Copy link
Collaborator Author

madcpf commented Oct 15, 2021

Everything looks great!

Thanks a lot Conrad! I tried to add back @pytest.mark.parametrize('board', BIG_CIRQ_BOARDS) in test_split_slide_merge_slide_coherence() but got the following DeviceMapping Error:
https://github.com/quantumlib/ReCirq/pull/210/checks?check_run_id=3908901393

Do you know why that happens?

Thanks!

@losos0
Copy link
Collaborator

losos0 commented Oct 16, 2021

I tried to add back @pytest.mark.parametrize('board', BIG_CIRQ_BOARDS) in test_split_slide_merge_slide_coherence() but got the following DeviceMapping Error: https://github.com/quantumlib/ReCirq/pull/210/checks?check_run_id=3908901393

Do you know why that happens?

It means the layout algorithm failed to find an acceptable mapping of the circuit onto the device graph. For some reason, in that case it just puts qubits that must be adjacent onto non-adjacent positions, so the error is surfaced a bit later.

@losos0
Copy link
Collaborator

losos0 commented Nov 2, 2021

I was trying to test how this change affects the number of moves which can be laid out on a device and found a bug where sometimes do_move forgets to return a value. I think it's coming from the (2+, 1) branch. You can check for parts without test coverage by doing python -m pip install pytest-cov and pytest --cov=. --cov-report=annotate and opening quantum_board.py,cover.

@madcpf
Copy link
Collaborator Author

madcpf commented Jan 10, 2022

ts that must be adjacent onto non-adjacent positions

I was trying to test how this change affects the number of moves which can be laid out on a device and found a bug where sometimes do_move forgets to return a value. I think it's coming from the (2+, 1) branch. You can check for parts without test coverage by doing python -m pip install pytest-cov and pytest --cov=. --cov-report=annotate and opening quantum_board.py,cover.

Thanks a lot Conrad for detecting the bug and sharing the way to show the test coverage! Bugs are fixed and test cases have been added.

Copy link
Collaborator

@losos0 losos0 left a comment

Choose a reason for hiding this comment

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

Could you squash the changes into 1 commit? I wanted to test, but didn't manage to - the branch is apparently old enough to not work with latest Cirq, and I couldn't rebase it onto the latest code due to all the merge commits. (I suggest using git rebase origin/master instead of git merge origin/master in the future to avoid this problem.)

@@ -509,6 +509,9 @@ def test_slide_with_two_path_qubits_coherence():
)


# @pytest.mark.parametrize("board", BIG_CIRQ_BOARDS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some detritus here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. It's removed.

@madcpf
Copy link
Collaborator Author

madcpf commented Jan 11, 2022

Everything looks great!

Thanks a lot Conrad!``

Could you squash the changes into 1 commit? I wanted to test, but didn't manage to - the branch is apparently old enough to not work with latest Cirq, and I couldn't rebase it onto the latest code due to all the merge commits. (I suggest using git rebase origin/master instead of git merge origin/master in the future to avoid this problem.)

Thanks Conrad. I tried to merge the past ~24 commits (which is relevant to the current effort) into one. I followed all steps in https://www.internalpointers.com/post/squash-commits-into-one-git, but it seems nothing has changed.

@losos0
Copy link
Collaborator

losos0 commented Jan 13, 2022

I managed to run it and on a 50-game sample, the modified code can lay out 1505/2423 moves in a 50-game sample, while the existing code gets 1447/2423. LGTM

@madcpf
Copy link
Collaborator Author

madcpf commented Jan 13, 2022

I managed to run it and on a 50-game sample, the modified code can lay out 1505/2423 moves in a 50-game sample, while the existing code gets 1447/2423. LGTM

Thanks a lot Conrad for testing it and sharing the results! I'm submitting this change now.

@madcpf madcpf merged commit d7559bf into quantumlib:master Jan 13, 2022
@madcpf madcpf deleted the second branch January 13, 2022 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimizations to reduce qubits based on non-entangled squares
4 participants