-
Notifications
You must be signed in to change notification settings - Fork 70
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
[WIP] Pass transpilation options #108
[WIP] Pass transpilation options #108
Conversation
Codecov Report
@@ Coverage Diff @@
## master #108 +/- ##
==========================================
+ Coverage 97.87% 97.92% +0.04%
==========================================
Files 7 7
Lines 283 289 +6
==========================================
+ Hits 277 283 +6
Misses 6 6
Continue to review full report at Codecov.
|
Hi @sagarpahwa, the scenarios you are testing in the PR comment look very thorough! They will make good unit tests. When you are ready for someone to take a look/review, let us know in the comments |
Hi @josh146 , I submitted PR for review. For testcases, I checked for BasicAer and Aer devices only. For IBMQ device, the time taken was increasing significantly and other testcases also seemed lightweight to me. |
@josh146 , should I re-trigger the build using an empty commit to make the tests pass? |
Hi @sagarpahwa! Some checks (e.g. the black formatter) need some adjustment at the moment. Feel free to ignore them, we'll update them dynamically. |
Hi @sagarpahwa, we've merged in an update for the A couple of small tweaks could help out with passing the currently failing CI checks: Documentation check / sphinx One thing to note is that docstrings left in the code need to adhere to the formatting conventions of
You can check the warnings locally by running Here is a guide on the installation of related packages. Formatting check / black It seems that a file needs reformatting as per the CI log ( This reformatting could be done by executing the following:
Note: you'll need to have codecov/project No worries on this one, if its status remains Expected. |
Thanks @antalszava , the formatting issues are resolved. |
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.
Hi @sagarpahwa, this is looking great! 🚀 🙂 Left a couple of suggestions. The main thing to reconsider would be related to the logic of updating the transpilation arguments after the device has been initialized.
…hwa/pennylane-qiskit into PassTranspilationOptions
Hi @antalszava , thanks for the thorough review. I made the amendments. |
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.
Hi @sagarpahwa! This is looking great, nice one! 😊 🎊 Left a couple of minor suggestions. but nothing major.
A couple of final steps for polishing:
- It can now be mentioned in the documentation that transpile options can be passed as
kwargs
to the device (e.g. the related sentence for the Aer and BasicAer devices could be slightly extended). - The very last step would be to update the CHANGELOG file by mentioning the addition and adding yourself as a contributor. I'll have to ask you to wait with this until around the early next week (at the moment we're in a feature freeze for PennyLane due to an upcoming release). We'll add a new changelog file, where this addition could be added. I'll leave a comment here regarding that.
Typos corrected. Co-authored-by: antalszava <antalszava@gmail.com>
Hi @sagarpahwa, almost there! 💯 Left a couple of small suggestions on the docs. You might have to pull before a new addition. There is now an updated |
Hi @antalszava, Sorry for the delay, my exams are going on. Thanks for such detailed pointers at each step. Those were really helpful in understanding the de facto standards of pennylane and open source frameworks in general. |
* Qiskit devices are now allowed to pass transpilation options. | ||
[(#108)](https://github.com/PennyLaneAI/pennylane-qiskit/pull/108) | ||
|
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.
💯
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.
Hi @sagarpahwa, that's okay, no problem! Sure thing 😊
Looks good to me, think this is good to be merged. Great addition! 🥇 🙂
[WIP] Pass transpilation options
Fixes #107
Summary
I tested the changes locally. The code is half baked. Need to add test cases. This commit is for initial review.
Here we are covering 4 possible scenarios. PFB the notebook snippets.
Case 1: transpile argument passed in QiskitDevice init
Case 2: transpile argument passed using apply
For this run only
Case 3: overriding transpile argument
For this run only
Case 4: overriding and persist transpile arguments