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

Add noise definition once after merging sequences. #206

Merged
merged 8 commits into from
Feb 20, 2020
Merged

Conversation

kylegulshen
Copy link
Contributor

@kylegulshen kylegulshen commented Dec 24, 2019

Description

The QVM is now performing a check on Kraus operators that was failing for the sum of RB Cliffords which each included a noisy gate definition; in particular, each gate definition added an identical copy of kraus operators to the noisy gate, which (I think) doesn't actually affect the implementation of the noise but does cause the check to fail. This PR simply adds the noisy gate to each Clifford and then adds the noisy gate definition once after these Cliffords have been combined to form the final sequences.

Checklist

  • The above description motivates these changes.
  • There is a unit test that covers these changes.
  • All new and existing tests pass locally and on Semaphore.

@kylegulshen kylegulshen requested a review from a team as a code owner December 24, 2019 17:22
Copy link

@appleby appleby left a comment

Choose a reason for hiding this comment

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

Without really understanding what's going on here, these changes look reasonable to me and I can confirm that the forest-benchmarking tests now pass (with one exception see ++ below) when run against qvm / quilc built from master.

Regarding whether duplicating the kraus operators affects the noise implementation: according to stylewarning in the slack thread I sent you an excerpt of, it doesn't matter for stochastic pure state evolution (the default for noisy qvm simulation methods), but would matter if you ran the qvm in density-matrix mode (via the --simulation-method full-density-matrix command line option).

++ I had to specify SKIP_SCS=1 to skip the test_diamond_norm_distance test, which otherwise failed for me. The one remaining failure was in test_tomography_process_nb, which also appears to call this diamond_norm_distance function, and the error message was similar to the one I get from test_diamond_norm_distance when I omit SKIP_SCS.

@kylegulshen
Copy link
Contributor Author

++ I had to specify SKIP_SCS=1 to skip the test_diamond_norm_distance test, which otherwise failed for me. The one remaining failure was in test_tomography_process_nb, which also appears to call this diamond_norm_distance function, and the error message was similar to the one I get from test_diamond_norm_distance when I omit SKIP_SCS.

It seems that there are sometimes issues with installing cvxpy and dependency scs. The solutions I've seen involve installing the libraries lapack and blas before installing cvxpy. The issue doesn't appear locally for me (or during the ci tests), so I'm not sure if this is something that needs fixing in the library or not.

@jlapeyre
Copy link
Contributor

jlapeyre commented Feb 14, 2020

It looks like this is a correct fix. But, it seems there is some semantic fuzziness that makes it easy for this kind of mistake to occur. Eg, a channel is sometimes called a "gate". And "noisy_gate" is used to describe a gate-independent noise channel. Maybe this can be cleaned up without breaking backwards compatibility.
EDIT: It looks like define_noisy_gate is intended to be used to create a channel that describes a particular noisy gate (by right-multiplying each Kraus operator by the clean gate matrix). Maybe calling this channel a noisy gate is ok afterall.

@jlapeyre
Copy link
Contributor

jlapeyre commented Feb 14, 2020

I agree with stylewarning via appleby. If too many Kraus operators are pushed into a noise channel, it no longer correctly describes state evolution (barring measurments). If sampling is done to apply stochastic errors in pure-state evolution, and the gates are only duplicated, then the sampling statistics don't change. But, all Kraus operators are applied when evolving a density matrix.

Or @kylegulshen s statement may have assumed that density-matrix evolution is not yet fully supported.

@jlapeyre jlapeyre merged commit ec84ea6 into master Feb 20, 2020
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.

3 participants