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 noisy channel: thermal relaxation #1766

Merged
merged 31 commits into from
Nov 3, 2021

Conversation

alejomonbar
Copy link
Contributor

@alejomonbar alejomonbar commented Oct 19, 2021

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    test directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the doc/releases/changelog-dev.md file, summarizing the
    change, and including a link back to the PR.

  • The PennyLane source code conforms to
    PEP8 standards.
    We check all of our code against Pylint.
    To lint modified files, simply pip install pylint, and then
    run pylint pennylane/path/to/file.py.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context:
Adding a new noisy channel which gives the thermal relaxation channel as part of the #971 issue. I got the information from the supplementary of Quantum classifier with tailored quantum kernel. I still need to figure out how to get the second case when T2 > T1.
Description of the Change:

Benefits:

Possible Drawbacks:

Related GitHub Issues:

Updating my master branch
I added the first case of the ThermalRelaxationError channel, it is still needed the second case when T_2 > T_1 but at this point I need help!
@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #1766 (22ef764) into master (085833d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1766   +/-   ##
=======================================
  Coverage   98.98%   98.98%           
=======================================
  Files         218      218           
  Lines       16375    16428   +53     
=======================================
+ Hits        16208    16261   +53     
  Misses        167      167           
Impacted Files Coverage Δ
pennylane/devices/default_mixed.py 100.00% <ø> (ø)
pennylane/ops/channel.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 085833d...22ef764. Read the comment docs.

@josh146
Copy link
Member

josh146 commented Oct 19, 2021

Thanks @alejomonbar for this nice contribution! 🙂 Let us know if you have any questions, or when this PR is ready for review, and someone from the team will be on hand to help.

@josh146 josh146 added the WIP 🚧 Work-in-progress label Oct 19, 2021
@josh146 josh146 changed the title Add noisy channel [WIP] Add noisy channel Oct 19, 2021
@alejomonbar
Copy link
Contributor Author

Thanks @alejomonbar for this nice contribution! 🙂 Let us know if you have any questions, or when this PR is ready for review, and someone from the team will be on hand to help.

Thank you @josh146. Indeed, I have some questions. For the case of T2 > T1, the usual approach is to map the Choi matrix into a Kraus operator. I'm wondering if I should create that mapping function as it is made for example in Qiskit and if that's the case, where!

@rmoyard rmoyard self-requested a review October 19, 2021 15:35
@rmoyard rmoyard changed the title [WIP] Add noisy channel [WIP] Add noisy channel: thermal relaxation Oct 19, 2021
@rmoyard
Copy link
Contributor

rmoyard commented Oct 19, 2021

Thanks @alejomonbar for this nice contribution! 🙂 Let us know if you have any questions, or when this PR is ready for review, and someone from the team will be on hand to help.

Thank you @josh146. Indeed, I have some questions. For the case of T2 > T1, the usual approach is to map the Choi matrix into a Kraus operator. I'm wondering if I should create that mapping function as it is made for example in Qiskit and if that's the case, where!

Thank you for opening this PR, I'll check the different documents and come back to you asap for the case T2 > T1!

@rmoyard
Copy link
Contributor

rmoyard commented Oct 19, 2021

@alejomonbar I checked the different documents about thermal relaxation. As PennyLane has no Choi representation of quantum channel, we indeed need to construct the kraus matrices from the Choi matrix given in the paper. Therefore for the case where dephasing is more prevalent than relaxation (T2>T1), you will need indeed need this transformation. For the moment you can create this function directly in channel.py and work there. I will check if a better location is needed.

Thank you again for your contribution and feel free to ask any question. Let me know when the PR will be ready for review!

@alejomonbar
Copy link
Contributor Author

@alejomonbar I checked the different documents about thermal relaxation. As PennyLane has no Choi representation of quantum channel, we indeed need to construct the kraus matrices from the Choi matrix given in the paper. Therefore for the case where dephasing is more prevalent than relaxation (T2>T1), you will need indeed need this transformation. For the moment you can create this function directly in channel.py and work there. I will check if a better location is needed.

Thank you again for your contribution and feel free to ask any question. Let me know when the PR will be ready for review!

Thank you, I will look at that and come back when I implement it!

antalszava and others added 3 commits October 20, 2021 09:48
Adding documentation of the two cases when T2 <= T1 and T2 > T1. It's left to make the test of this noisy model.
I add a test for the ThermaRelaxationError and improve the Kraus operators for T2 > T1. Now, I don't use the Choi matrix and extract the eigenvalues and eigenvectors, instead, I use the parametric representation of those eigenvalues and eigenvectors.
@alejomonbar
Copy link
Contributor Author

@antalszava I improve the code and now it gives results for both cases, T2 <= T1 and T2 > T1, the second case needs the Choi-matrix eigenvalues and eigenvectors, I put the parametric representation of those values. Let me know if you have any comments. Also, I added a test for the new ThermalRelaxation error.

@rmoyard
Copy link
Contributor

rmoyard commented Oct 21, 2021

@antalszava I improve the code and now it gives results for both cases, T2 <= T1 and T2 > T1, the second case needs the Choi-matrix eigenvalues and eigenvectors, I put the parametric representation of those values. Let me know if you have any comments. Also, I added a test for the new ThermalRelaxation error.

@alejomonbar Thank you, I'll give a look to the PR today and will give some advice!

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, before I can give a full review you have different points to work on:

  1. Codefactor: in the folder channel.py minor things like trailinng white spaces and too many statements should be solved easily.
  2. You can pip install black and run black -l 100 your_file.py on all the changed files.
  3. For docstrings and documentation, there probably some small errors of formatting.
  4. Some tests are failing, like test_kraus_matrices_sum_identitydid you check it locally? Maybe there is small mistake in the Kraus operators or you need to adapt the test for the case. Same for the new tests are failing as well, do they pass locally for you?

@alejomonbar
Copy link
Contributor Author

Thank you for the contribution, before I can give a full review you have different points to work on:

  1. Codefactor: in the folder channel.py minor things like trailinng white spaces and too many statements should be solved easily.
  2. You can pip install black and run black -l 100 your_file.py on all the changed files.
  3. For docstrings and documentation, there probably some small errors of formatting.
  4. Some tests are failing, like test_kraus_matrices_sum_identitydid you check it locally? Maybe there is small mistake in the Kraus operators or you need to adapt the test for the case. Same for the new tests are failing as well, do they pass locally for you?

Thank you @rmoyard , I tried it but I have a problem when trying to run the test I got the following message, ModuleNotFoundError:No module named 'autoray' and I installed it using pip install autoray, and looks like it is installed, but still, I got the same message.

@rmoyard
Copy link
Contributor

rmoyard commented Oct 21, 2021

Thank you for the contribution, before I can give a full review you have different points to work on:

  1. Codefactor: in the folder channel.py minor things like trailinng white spaces and too many statements should be solved easily.
  2. You can pip install black and run black -l 100 your_file.py on all the changed files.
  3. For docstrings and documentation, there probably some small errors of formatting.
  4. Some tests are failing, like test_kraus_matrices_sum_identitydid you check it locally? Maybe there is small mistake in the Kraus operators or you need to adapt the test for the case. Same for the new tests are failing as well, do they pass locally for you?

Thank you @rmoyard , I tried it but I have a problem when trying to run the test I got the following message, ModuleNotFoundError:No module named 'autoray' and I installed it using pip install autoray, and looks like it is installed, but still, I got the same message.

Does it appear in the environment if you write pip list?

@alejomonbar
Copy link
Contributor Author

Thank you for the contribution, before I can give a full review you have different points to work on:

  1. Codefactor: in the folder channel.py minor things like trailinng white spaces and too many statements should be solved easily.
  2. You can pip install black and run black -l 100 your_file.py on all the changed files.
  3. For docstrings and documentation, there probably some small errors of formatting.
  4. Some tests are failing, like test_kraus_matrices_sum_identitydid you check it locally? Maybe there is small mistake in the Kraus operators or you need to adapt the test for the case. Same for the new tests are failing as well, do they pass locally for you?

Thank you @rmoyard , I tried it but I have a problem when trying to run the test I got the following message, ModuleNotFoundError:No module named 'autoray' and I installed it using pip install autoray, and looks like it is installed, but still, I got the same message.

Does it appear in the environment if you write pip list?

Yes, it does with version autoray 0.2.5

@rmoyard
Copy link
Contributor

rmoyard commented Oct 21, 2021

@alejomonbar Could you give me your system information? import pennylane as qml; qml.about()

@alejomonbar
Copy link
Contributor Author

@alejomonbar Could you give me your system information? import pennylane as qml; qml.about()

No, I cannot import pennylane as qml, I got the same message ModuleNotFoundError: No module named 'autoray'

I solve the problems with the tests that were causing errors.
@alejomonbar
Copy link
Contributor Author

@alejomonbar In the pennylane/devices/default_mixed.py file you can find a list of operations/channels and you have to add ThermalRelaxationError channel there!

@rmoyard Thank you. Now, all the tests pass, let me know if you have any questions about what I did

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

@alejomonbar You have to do multiple things to pass all the CI checks:

  1. Codefactor: trailing whitespaces issues
  2. Codefactor: unecessary else after return
  3. Codecov: you need to add tests for line 557/559/561, basically just test that the error messages are raised.
  4. Two issues with docstrings: /github/workspace/pennylane/ops/channel.py:docstring of pennylane.ops.channel.ThermalRelaxationError:48: WARNING: Unexpected indentation. /github/workspace/pennylane/ops/channel.py:docstring of pennylane.ops.channel.ThermalRelaxationError:60: WARNING: Definition list ends without a blank line;

@alejomonbar
Copy link
Contributor Author

@alejomonbar You have to do multiple things to pass all the CI checks:

  1. Codefactor: trailing whitespaces issues
  2. Codefactor: unecessary else after return
  3. Codecov: you need to add tests for line 557/559/561, basically just test that the error messages are raised.
  4. Two issues with docstrings: /github/workspace/pennylane/ops/channel.py:docstring of pennylane.ops.channel.ThermalRelaxationError:48: WARNING: Unexpected indentation. /github/workspace/pennylane/ops/channel.py:docstring of pennylane.ops.channel.ThermalRelaxationError:60: WARNING: Definition list ends without a blank line;

@rmoyard I have problems with make docs, maybe that's why I cannot pass this test. When I try to run it, I get the following message AttributeError: 'Sphinx' object has no attribute 'add_stylesheet'. In any case, I tried to follow your comments.

alejomonbar and others added 2 commits October 26, 2021 14:57
Adding tests for the errors when the times t1, t2, and tg are negative.
@rmoyard
Copy link
Contributor

rmoyard commented Oct 27, 2021

@alejomonbar If you solve the two doc issues mentioned in my previous comment (line 48 unexpected indentation and line 60 blank line), the doc tests should pass.

@rmoyard rmoyard removed the WIP 🚧 Work-in-progress label Oct 27, 2021
@rmoyard rmoyard changed the title [WIP] Add noisy channel: thermal relaxation Add noisy channel: thermal relaxation Oct 27, 2021
@rmoyard
Copy link
Contributor

rmoyard commented Oct 27, 2021

@alejomonbar Thank you for the work, I'll give a full review asap!

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks @alejomonbar, it looks good overall. I've left some comments, let me know if you have some questions.

pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Show resolved Hide resolved
pennylane/ops/channel.py Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
tests/ops/test_channel_ops.py Outdated Show resolved Hide resolved
alejomonbar and others added 2 commits October 29, 2021 07:34
Thank you! @rmoyard

Co-authored-by: Romain <rmoyard@gmail.com>
Adding the suggestions of @rmoyard about the reshaping of the eigvecs and adding a test for t2 > t1 to the TestChannels. Additionally, adding the new ThermalRelaxationError to changelog-dev
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
@rmoyard rmoyard added the hacktoberfest-accepted Hacktoberfest not merged but in a good shape label Oct 29, 2021
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

It looks good to me, last comments for Code factor errors and also a dosctring. After that it should be all good 💯

pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
pennylane/ops/channel.py Outdated Show resolved Hide resolved
@josh146 josh146 merged commit dfc5741 into PennyLaneAI:master Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest not merged but in a good shape
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants