Add amplitude damping noise model (#497)#540
Conversation
|
Hi, kindly review my PR, I have added a few other tests, testing things that the other PRs didn't seem to address, primarily, evaluating the contribution of each of the noise sources(dephasing and damping) together by running them in the same circuit. I am open to any comments and reviews on this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #540 +/- ##
==========================================
+ Coverage 89.18% 89.30% +0.11%
==========================================
Files 49 50 +1
Lines 7354 7430 +76
==========================================
+ Hits 6559 6635 +76
Misses 795 795 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…tel24/graphix into amplitude-damping-noise
|
Hi @NandanPatel24 , thanks for your contribution to Graphix ! Please take a look at the failing ruff and typing checks in CI. Also, Codecov has detected that some lines in your code are not tested. Please include these in your test suite as well. Your implementation looks correct, and I like the new tests you've added compared to previous PRs. However, something we would really like to do in the test suite is directly comparing the result of a pattern simulation with a random noise probability Similarly, we would like to test the resulting density matrix from applying amplitude damping noise at the preparation, entanglement, measurement, and correction steps. This requires computing the state analytically at each step of the pattern. Do you think you could add this to your tests? |
Hi, I have added the tests that you gave, for both the H and the RZ gates, as well as added the damping at 4 different steps and compared it with the analytical solution |
|
Thanks for the very quick response! Please take a look at the CI as ruff and typecheck are still failing. I will submit a review of your additions as soon as possible. |
pranav97nair
left a comment
There was a problem hiding this comment.
Hi @NandanPatel24 , nice job with this PR! It looks like the CI is still failing due to some ruff issues on tests/test_noise_model.py. Specifically, I believe you need to add an empty line after line 233 and sort the imports at the top of the file.
In addition, please take a look at my specific feedback below. Once you have addressed the mentioned points, you would be on track to be assigned this issue.
Hi, I have implemented the changes that you recommended, have a look at those once, and the ruff and CI issues should now be fixed |
|
Hi @pranav97nair, please provide your reviews |
There was a problem hiding this comment.
Hi @NandanPatel24 , thanks for your response! I am overall quite satisfied with your code, and would like to go ahead and assign you the issue. To become an eligible assignee, please first comment on Issue #497 with a reference to your PR, preferably today.
I have however requested some minor changes, so please take a look at them below. To ensure you are replicating the CI type-checks properly in your local environment, take a look at the configuration in .github/workflows/typecheck.yml.
Also, note that we have a two‑approval rule for non‑trivial PRs: therefore, once I or another maintainer approves this PR, we'll need a second approval before merging.
|
Congratulations on being assigned Issue #497 for UnitaryHack, @NandanPatel24 ! You should receive the bounty once we merge this PR and close the issue. |
|
Hi @pranav97nair , I have made the required changes in commit c3f8c7c |
pranav97nair
left a comment
There was a problem hiding this comment.
Hi @NandanPatel24 , please take a look at the below comments. In addition to addressing these, can you also update the CHANGELOG.md with an entry at the top briefly summarizing what is being added in this PR ?
…tel24/graphix into amplitude-damping-noise
|
Hi @pranav97nair, I have changed the suggested code snippets, and also updated CHANGELOG.md, do you have any more suggested changes? |
pranav97nair
left a comment
There was a problem hiding this comment.
Hi @NandanPatel24 , thanks a lot for your prompt responses to my feedback. I do want to ask for one final update from my side, which is to include AmplitudeDampingNoiseModel in graphix/__init__.py so that this noise model can be directly imported from the graphix namespace.
Other than that, the PR looks good to me now so I will approve. We will wait for one more approval before merging, as I mentioned before.
…tel24/graphix into amplitude-damping-noise
Sure, I added that to the init.py file, will wait for the other approval |
thierry-martinez
left a comment
There was a problem hiding this comment.
Good job, thanks! I just have some minor comments.
|
|
||
| # copy of initial dm | ||
| rho_test = dm.rho | ||
| rho_test = np.asarray(dm.rho, dtype=np.complex128) |
There was a problem hiding this comment.
Why do we need to change this?
There was a problem hiding this comment.
The reason for that is the plain dm.rho is typed object_ | complex128, so k1 @ rho_test (with k1 being complex128) fails mypy. Hence I switched to dm.rho.astype(np.complex128, copy=False) to fix the type without copying.
There was a problem hiding this comment.
I don't observe any errors from either mypy or pyright when I revert that line to
| rho_test = np.asarray(dm.rho, dtype=np.complex128) | |
| rho_test = dm.rho |
Could you check again on your side? Moreover, this code was already in master, which means the CI has already validated it without any typing errors.
thierry-martinez
left a comment
There was a problem hiding this comment.
You've my approval! Thank you for your contribution.
Summary
Closes #497.
Changes
graphix/channels.py — adds amplitude_damping_channel(prob) and two_qubit_amplitude_damping_channel(prob), returning KrausChannel objects. Single-qubit operators are K₁ = diag(1, √(1−γ)) and K₂ = [[0, √γ], [0, 0]]; the two-qubit version is the tensor product of two independent single-qubit channels (four operators {Kᵢ⊗Kⱼ}). Unlike the existing Pauli-based channels these operators aren't scalar multiples of Pauli matrices, so they're built explicitly with coef=1.0.
graphix/noise_models/amplitude_damping.py (new) — defines AmplitudeDampingNoise, TwoQubitAmplitudeDampingNoise (both deriving from Noise), and AmplitudeDampingNoiseModel (deriving from NoiseModel). The model mirrors DepolarisingNoiseModel's command-injection logic. confuse_result returns the result unchanged, since amplitude damping is a purely quantum channel with no classical readout-error component; readout error can be added by composing with another model via ComposeNoiseModel. (This is why the model omits the measure_error_prob parameter that the depolarising model carries.)
graphix/noise_models/init.py — registers the three new classes for top-level import, alongside the depolarising exports.
A convention note
Tests and soundness
The suite is split between proving the channel matches the Kraus formula (random-state and by-hand-sum tests) and proving it is amplitude damping specifically (deterministic basis-state tests exploiting the channel's asymmetry — these are the soundness argument the issue asks for).
tests/test_kraus.py
tests/test_density_matrix.py(https://github.com/TeamGraphix/graphix/blob/master/tests/test_density_matrix.py)
tests/test_noise_model.py((https://github.com/TeamGraphix/graphix/blob/master/tests/test_noise_model.py)
All checks pass locally (ruff, ruff-format, mypy, pyright, pytest).
Per unitaryHACK's AI-use guidelines: AI assistance was used while developing this contribution, especially to format this PR content, and for debugging. All ideas remain my own