Conversation
- Fixed `_parser.py` formatting issues.
- Added testers for `cp`, `c3x`, `c4x`, `csx`, and `c3sqrtx` gates. - Fixed u0 gate. - Removed `r`, `ryy`, `cu2`, and `iswap` as they are not part of qelib 1.
- Fixes gate definition for `id` to address git-blame note. - Fixes gate definition for `p` to use ZPowGate. - Adds rccx and rc3x using `cirq.MatrixGate`. - Adds missing testers for cry and crz.
- Fixed failed CIs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7752 +/- ##
========================================
Coverage 99.62% 99.62%
========================================
Files 1106 1109 +3
Lines 99421 99563 +142
========================================
+ Hits 99046 99188 +142
Misses 375 375 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I added single qubit state prep manually, it should be removed when cirq gets state preparation added. I'll be sure to remind in the next cirq cynq. |
|
I need to check a few things with colleagues; I will have an update early next week. |
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
Please see inline comments. Otherwise LGTM, thank you for contributing this.
PS: I'd like to loop in @tanujkhattar for more comments after we converge on the initial review.
| state = np.random.rand(2**N) + 1j * np.random.rand(2**N) | ||
| state /= np.linalg.norm(state) |
There was a problem hiding this comment.
Please reuse cirq.testing.random_superposition instead
There was a problem hiding this comment.
I checked, and random_superposition can have volume-law states which are hard to approximate with vanilla MPS (I'll add the sweeping I mentioned before soon, not in this PR though). Is it alright that for this test I keep it as is since the one I have there produces area-law states?
There was a problem hiding this comment.
I guess we can leave it as is for the initial version.
I am a bit surprised that the tested state vector seems to need coefficients with only positive real and imaginary components; the test fails if some are allowed to be negative (patch below)
Is that an expected behavior for MPS encoding?
cc @tanujkhattar
diff --git a/cirq-core/cirq/contrib/mps_synthesis/mps_sequential_test.py b/cirq-core/cirq/contrib/mps_synthesis/mps_sequential_test.py
index 2ec1b90a4..f3b1c899d 100644
--- a/cirq-core/cirq/contrib/mps_synthesis/mps_sequential_test.py
+++ b/cirq-core/cirq/contrib/mps_synthesis/mps_sequential_test.py
@@ -35 +35 @@ def test_compile_area_law_states(num_qubits: int) -> None:
- state = np.random.rand(2**num_qubits) + 1j * np.random.rand(2**num_qubits)
+ state = 2 * np.random.rand(2**num_qubits) - 1 + 1j * np.random.rand(2**num_qubits)
…s internal module only. - Added assertion to `_gram_schmidt` to raise an error should `matrix` not be square to catch edge cases not known at the time of changing this code. - Changed class name to `MPSSequential`. - Added class docstring. - Removed `convention` from class attributes. - Added indentation to docstring sections where needed. - Removed `logger` uses. - Added `mps_circuit_from_statevector` to `__init__.py` for ease of use. - Used `cirq.testing.random_superposition` in testers where possible, except `test_compile_area_law_states` which requires strictly area-law entangled states as part of the test. - Rewrote `test_compile_trivial_state_with_mps_pass` to avoid using qasm and avoid inter-module dependency. - Used `np.testing.assert_allclose` for about exact matching assertions.
|
Pushed commit that addressed as many comments as I could. 4 comments remain which need your kind insight. Main comment remaining is converging on optimal/preferred implementation for |
|
@pavoljuhas Greetings sir, Hope you are well. Have you had a chance to review the new additions? |
I'll have the review in later today. Sorry about the delay. |
Let us stick to the present pattern. No change in code function.
Address missing coverage. `_gram_schmidt` is module-local and should not be used as public API.
Per `_gram_schmidt` type annotation the matrix has True iscomplexobj.
These are already present in the function signature.
No change in the effective code.
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
I have pushed in a couple of small clean ups which should unblock the failing coverage. Looks OK for my part, but I'd like to have another check by @tanujkhattar.
Tanuj - could you please give this a quick look?
|
@tanujkhattar Gentle ping … |
mhucka
added
triage/discuss
|
I will help review. |
|
@wjhuggins - gentle ping, can you PTAL? |
5 participants
Closes #7650 .