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

Use matrix definitions from pyquil.gate_matrices. Issues #99 and #16 #103

Merged
merged 4 commits into from
May 1, 2019

Conversation

kylegulshen
Copy link
Contributor

@kylegulshen kylegulshen commented Apr 30, 2019

Issues #99 and #16

@kylegulshen kylegulshen requested a review from a team as a code owner April 30, 2019 17:04
Copy link
Contributor

@joshcombes joshcombes left a comment

Choose a reason for hiding this comment

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

I have one comment which you can implement or ignore. It is about possibly reinstating the amplitude_damping in the Kraus representation.

I found some more places where you can use these definitions eg. in test_distance_measures the functions test_diamond_norm, test_watrous_bounds, test_process_fidelity can use this treatment. I made the changes feel free to revert them and do it a different way.

Otherwise it looks good.

from pyquil import Program
from pyquil import gate_matrices as mat
from pyquil.api import QVM
from pyquil.gates import CNOT, X
from pyquil.gate_matrices import X as X_MAT, Y, Z
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I had not thought about this problem i.e.

from pyquil.gates import CNOT, X
from pyquil.gate_matrices import X as X_MAT, Y, Z

maybe we should standardize around your solution above i.e. the matrix representation is always called G_MAT for some gate G.

# Test philosophy:
# Using the by hand calculations found in the docs we check conversion
# between one qubit channels with one Kraus operator (Hadamard) and two
# Kraus operators (the amplitude damping channel). Additionally we check
# a few two qubit channel conversions to get additional confidence.

# The Amplitude Damping channel (one qubit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm Im in superposition about the removal here. Here are my thoughts.

  1. pyQuil noise models are in flux now. We don't know what will happen.
  2. this is self contained within forest.benchmarking so keeping it makes it easy to check with conventions in the markdown file.
  3. the name relaxation does not match what we have in the docs and the rest of the code.
  4. I get the flip side too. Reducing code duplication etc.

I guess in the end my superposition is slightly biased towards restoring the function but I'll leave it up to you.

@joshcombes joshcombes mentioned this pull request May 1, 2019
4 tasks
@kylegulshen kylegulshen merged commit ba2b5e0 into master May 1, 2019
@kylegulshen kylegulshen deleted the pyquil-gate-defs-99 branch May 1, 2019 16:54
@karalekas karalekas added this to the v0.4 milestone May 3, 2019
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