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

ECR implementation #2613

Merged
merged 47 commits into from
Jun 8, 2022
Merged

ECR implementation #2613

merged 47 commits into from
Jun 8, 2022

Conversation

avbhadw
Copy link
Contributor

@avbhadw avbhadw commented May 25, 2022

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: Implementing ECR function as described here

Description of the Change: adding the operation (adjoint, eigenvalues, decomposition), adding support for qubit devices

Benefits:

Possible Drawbacks:

Related GitHub Issues:
Closes #2581

@antalszava
Copy link
Contributor

Hi @avbhadw, thank you for the PR! 🙂 Let us know if this would be ready for review.

@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #2613 (e645941) into master (d31b6f7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2613   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files         249      249           
  Lines       20202    20220   +18     
=======================================
+ Hits        20119    20137   +18     
  Misses         83       83           
Impacted Files Coverage Δ
pennylane/devices/default_mixed.py 100.00% <ø> (ø)
pennylane/devices/default_qubit.py 100.00% <ø> (ø)
pennylane/ops/qubit/__init__.py 100.00% <ø> (ø)
pennylane/ops/qubit/non_parametric_ops.py 100.00% <100.00%> (ø)
pennylane/transforms/commutation_dag.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 d31b6f7...e645941. Read the comment docs.

@avbhadw
Copy link
Contributor Author

avbhadw commented May 25, 2022

Hey @antalszava I think this is ready for review. I did implement a function to calculate the power of the matrix but I am not sure how to test it, so I removed it. In my new commit there is no power function but when I run
python -m pytest tests/ops/qubit/test_non_parametric_ops.py
6 tests are failing, I think it is because this matrix uses complex numbers. Should I add back the power function?

Another issue I have is running make docs. I get the error below:
Extension error: You must configure the bibtex_bibfiles setting make[1]: *** [html] Error 2 make: *** [docs] Error 2

Is there a bibtex file I need to add?

Thanks!

@antalszava antalszava self-requested a review May 31, 2022 16:36
Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Hi @avbhadw, apologies for the delayed response here! I've been a bit unwell for the past couple of days.

The solution is looking great overall! 🎉 🙂 I've left a couple of minor comments.

For your questions:

  1. The power method should be tested already based on your addition (see the related comment in the test file);
  2. What if the sphinxcontrib-bibtex package is installed via pip install sphinxcontrib-bibtex.

Also, there seem to be some CI checks failing (apart form the test runners). Let me know and I could help with these if need be.

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/devices/tests/#test_gates.py# Outdated Show resolved Hide resolved
tests/test_operation.py Outdated Show resolved Hide resolved
pennylane/ops/qubit/non_parametric_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit/non_parametric_ops.py Show resolved Hide resolved
pennylane/ops/qubit/__init__.py Outdated Show resolved Hide resolved
tests/ops/qubit/test_non_parametric_ops.py Show resolved Hide resolved
tests/ops/qubit/test_non_parametric_ops.py Show resolved Hide resolved
pennylane/ops/qubit/non_parametric_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit/non_parametric_ops.py Outdated Show resolved Hide resolved
avbhadw and others added 15 commits June 1, 2022 18:53
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
@avbhadw avbhadw requested a review from antalszava June 1, 2022 23:41
@avbhadw
Copy link
Contributor Author

avbhadw commented Jun 3, 2022

Hey @antalszava

I think I have resolved almost all of the reviews you have suggested(one unresolved, see above). I was having some trouble running make coverage (my computer does not have enough RAM for this to work), but I borrowed someone else's laptop (OS: Linux) and it worked. I believe I am no longer failing CI checks at least locally.

After trying your suggestion, I still have the same error with sphinxcontrib-bibtex. Another issue I have is installing the requirements to run make docs. I am using a MacOS and I get this error whenever I try to install torch
ERROR: Could not find a version that satisfies the requirement torch==1.9.0+cpu (from versions: 1.4.0, 1.5.0, 1.5.1, 1.6.0, 1.7.0, 1.7.1, 1.8.0, 1.8.1, 1.9.0, 1.9.1, 1.10.0, 1.10.1, 1.10.2, 1.11.0) ERROR: No matching distribution found for torch==1.9.0+cpu

From what I understand there isn't a torch package 1.9.0+cpu for macOS, I did remove the +cpu and install torch but I had an error when running make tests (short summary):

FAILED tests/docs/test_supported_confs.py::TestSupportedConfs::test_all_backprop_none_shots[wire_specs0-DensityMatrix-torch] - RuntimeError: unsupported input to tensordot, got dims=0 FAILED tests/docs/test_supported_confs.py::TestSupportedConfs::test_all_backprop_finite_shots[wire_specs0-DensityMatrix-torch] - RuntimeError: unsupported input to tensordot, got dims=0

I think once I am able to successfully run pip install -r doc/requirements.txt, the bibtex error should resolve.

I am thinking of trying to use docker but I am not sure if it would solve the problem.

avbhadw and others added 4 commits June 3, 2022 13:24
@avbhadw avbhadw requested a review from albi3ro June 3, 2022 17:36
Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Hi @avbhadw, this looks good! 🎉

1-2 minor suggestions and it seems that the formatting is off (running black should help), afterwards, we'd merge this one.

As for the configuration issues, I'm not sure unfortunately. 🤔 Perhaps Docker would work, we do get some issues with Docker at times. For this PR, building the docs should be fine for now, the CI check seems good. 🙂

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/ops/qubit/non_parametric_ops.py Outdated Show resolved Hide resolved
tests/ops/qubit/test_non_parametric_ops.py Outdated Show resolved Hide resolved
avbhadw and others added 3 commits June 3, 2022 14:35
Co-authored-by: antalszava <antalszava@gmail.com>
Co-authored-by: antalszava <antalszava@gmail.com>
@avbhadw
Copy link
Contributor Author

avbhadw commented Jun 3, 2022

Hey @antalszava

I have implemented your suggestions and ran black (sorry I forgot). Thanks for being patient with me!

@avbhadw
Copy link
Contributor Author

avbhadw commented Jun 6, 2022

Hey @antalszava

I noticed black failing. I fixed it and pushed back up.
Sorry for the inconvenience

@antalszava
Copy link
Contributor

Hi @avbhadw, no worries! 🙂 Thank you.

I'll be merging this PR soon. Thank you for your contribution! 👏

@antalszava antalszava merged commit 189de28 into PennyLaneAI:master Jun 8, 2022
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.

Adding ECR operation
4 participants