-
Notifications
You must be signed in to change notification settings - Fork 41
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
Refactor gate operations and add a new kernel for single qubit gates #202
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
==========================================
+ Coverage 99.64% 99.65% +0.01%
==========================================
Files 4 4
Lines 278 293 +15
==========================================
+ Hits 277 292 +15
Misses 1 1
Continue to review full report at Codecov.
|
Test Report (C++) on Ubuntu 1 files ± 0 1 suites ±0 0s ⏱️ ±0s Results for commit baf81a0. ± Comparison against base commit 7515848. This pull request removes 287 and adds 193 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Hi all, I applied most of the changes from the code review. For a few remaining issues, check my comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work again @chaeyeunpark
I'd love to see a graph dropped here with the performance comparison of this PR against the current master using the kernel tests, as well as some workflow tests.
I think this is pretty much ready to approve (given the above, and the last few comments I made). I reckon it will be worth updating the architecture images and discussion on the docs in a following PR to account for the new structure.
Typo fixed Co-authored-by: Lee James O'Riordan <[email protected]>
Co-authored-by: Lee James O'Riordan <[email protected]>
Co-authored-by: Lee James O'Riordan <[email protected]>
…htning into refac_gate_ops
For some reason, the docs do not generate the inheritance diagram https://pennylane-lightning--202.org.readthedocs.build/en/202/code/__init__.html#class-inheritance-diagram I wonder if we need to explicitly install graphviz/dot or make some changes to the documentation build for this. |
Hi @mlxd, I added |
Aha. It was |
Great work on finding this! Happy to see this merged from my side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
85 files reviewed 😄 nothing else to be added. Great job!
Once again, great work on this @chaeyeunpark |
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
tests
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
.github/CHANGELOG.md
file, summarizing thechange, and including a link back to the PR.
Ensure that code is properly formatted by running
make format
.When all the above are checked, delete everything above the dashed
line and fill in the pull request template.
Context: Previous implementation of gate operations is not well-performing. Thus it has been discussed to add an alternative implementation. This PR proposes a possible way to do it.
Description of the Change: Now, one can add an alternative kernel (gate implementations) relatively easily. I have also added a kernel with less memory bandwidth, which is faster for most gates.
Benefits: Faster gates. One can also add alternative kernels rather easily.
Possible Drawbacks: Some parts of the code get a bit complicated. Especially, template metaprogramming was unavoidable at some points. I hope to replace some of them in C++20.
Related GitHub Issues:
Sorry for making this giant PR...