-
Notifications
You must be signed in to change notification settings - Fork 40
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 the simulate class for the new device #817
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## kokkosNewAPI_backend #817 +/- ##
=========================================================
+ Coverage 64.27% 90.01% +25.74%
=========================================================
Files 18 19 +1
Lines 1702 2034 +332
=========================================================
+ Hits 1094 1831 +737
+ Misses 608 203 -405 ☔ View full report in Codecov by Sentry. |
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.
I'm observing several coverage complaints coming from Codecov. Some of them might be a "codecov fluke", some of them might not. @LuisAlfredoNu, could you please double-check and let us know when the coverage is OK (even if codecov is complaining). You might want to rerun all CIs. You can use:
git commit -m "trigger CIs" --allow-empty
git push -u origin kokkosNewAPI_simulate
or any variant of that.
I just noticed that there is also a formatting complaint. |
Checking why codecov does not show the increase in the coverage of the changes I conclude the following:
To solve this issue I comment the lines that run the test from Pennyline repo in the |
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.
Nice work @LuisAlfredoNu.
Co-authored-by: Vincent Michaud-Rioux <[email protected]>
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.
Thanks @LuisAlfredoNu . A few more suggestions/comments, but this is almost ready to merge IMO.
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.
Nice work @LuisAlfredoNu! Just a few Q.
Co-authored-by: Vincent Michaud-Rioux <[email protected]>
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.
Nice work! 💯
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
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.
Nice work, @LuisAlfredoNu!
Thank you, @PennyLaneAI/performance, for your very thorough review.
This is the type of high-quality work (review and dev) the whole team consistently delivers!
I resolved some apparent complete conversations, please feel free to re-open if sensible.
Regarding overall problems with coverage, or updates that would apply to LQubit too, let's keep attention to it and come back later if having time.
@LuisAlfredoNu, keep notes of suggestions the very nice suggestions here. They will be very useful after we have our MVP in place.
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.
Nice work @LuisAlfredoNu . I'll approve while leaving a suggestion. Feel free to resolve as you wish. Cheers!
Co-authored-by: Vincent Michaud-Rioux <[email protected]>
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 job! @LuisAlfredoNu Happy to approve!
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:
Migrate the lightning.kokkos device to the new device API.
Description of the Change:
The 'simulate' method is necessary to achieve full functionality with the new device API.
We are going to follow the same recipe used with LightningQubit.
Benefits:
Add one of the essential methods for the new device API into LightningKokkos.
Possible Drawbacks:
Related GitHub Issues:
Partial / Freezzed PR⚠️ ❄️
To make a smooth integration of LightningKokkos with the new device API, we set the branch
kokkosNewAPI_backend
as the base branch target for this development.The branch
kokkosNewAPI_backend
has the mock of all classes and methods necessary for the new API. Also, several tests were disabled withAdditionally, the CI testing from PennyLane for LKokkos is temporally disabled through commenting the following lines in
.github/workflows
filesHowever, these tests will unblocked as the implementation progresses.
After all the developments for integrating LightningKokkos with the new API have been completed then the PR will be open to merge to master
[sc-68801] [sc-68816] [sc-68819]