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

[WIP] Pass transpilation options #108

Merged
merged 17 commits into from
Oct 24, 2020

Conversation

sagarpahwa
Copy link
Contributor

[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.

import numpy as np
import pennylane as qml
from pennylane_qiskit import *

Case 1: transpile argument passed in QiskitDevice init

dev = AerDevice(wires=2, test_param='dummy_param', optimization_level=3, validate=False)
def init_state(n):
    state = np.random.random([2 ** n]) + np.random.random([2 ** n]) * 1j
    state /= np.linalg.norm(state)
    return state
state = init_state(2)
state
array([0.21517941+0.23902292j, 0.04778645+0.44990237j,
       0.60157048+0.01456144j, 0.57077425+0.06314949j])

Case 2: transpile argument passed using apply

For this run only

dev.apply([qml.QubitStateVector(state,wires=[0,1])], routing_method='stochastic', seed_transpiler=22)
cheking transpile args:  {'optimization_level': 3}
cheking temp transpile args:  {'routing_method': 'stochastic', 'seed_transpiler': 22}
cheking merged transpile args:  {'optimization_level': 3, 'routing_method': 'stochastic', 'seed_transpiler': 22}

Case 3: overriding transpile argument

For this run only

dev.apply([qml.QubitStateVector(state,wires=[0,1])], optimization_level=2
cheking transpile args:  {'optimization_level': 3}
cheking temp transpile args:  {'optimization_level': 2}
cheking merged transpile args:  {'optimization_level': 2}

Case 4: overriding and persist transpile arguments

dev.apply([qml.QubitStateVector(state,wires=[0,1])], optimization_level=2, persist_transpile_options=True)
cheking transpile args:  {'optimization_level': 2}
cheking temp transpile args:  {}
cheking merged transpile args:  {'optimization_level': 2}

@codecov
Copy link

codecov bot commented Oct 7, 2020

Codecov Report

Merging #108 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
pennylane_qiskit/aer.py 100.00% <ø> (ø)
pennylane_qiskit/basic_aer.py 100.00% <ø> (ø)
pennylane_qiskit/qiskit_device.py 97.51% <100.00%> (+0.09%) ⬆️

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 28064a9...74a52d3. Read the comment docs.

@josh146
Copy link
Member

josh146 commented Oct 7, 2020

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

@sagarpahwa sagarpahwa marked this pull request as ready for review October 7, 2020 23:53
@sagarpahwa
Copy link
Contributor Author

sagarpahwa commented Oct 8, 2020

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.

@sagarpahwa
Copy link
Contributor Author

@josh146 , should I re-trigger the build using an empty commit to make the tests pass?

@antalszava
Copy link
Contributor

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.

@antalszava
Copy link
Contributor

antalszava commented Oct 9, 2020

Hi @sagarpahwa, we've merged in an update for the black formatting check and now it should be working fine! That means that the checks should now be good to go.

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 rst markup (linking the relevant part of the development guide on documentation). This involves fixing up warnings that are raised by sphinx, such as:

/pennylane-qiskit/pennylane_qiskit/qiskit_device.py:docstring of pennylane_qiskit.IBMQDevice.apply:4: WARNING: Unexpected indentation.

You can check the warnings locally by running make docs from the root folder of PennyLane-Qiskit.

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 (/home/runner/work/pennylane-qiskit/pennylane-qiskit/pennylane_qiskit/qiskit_device.py).

This reformatting could be done by executing the following:

black -l 100 pennylane_qiskit/qiskit_device.py

Note: you'll need to have black installed using pip locally.

codecov/project

No worries on this one, if its status remains Expected.

@sagarpahwa
Copy link
Contributor Author

Thanks @antalszava , the formatting issues are resolved.

@antalszava antalszava self-requested a review October 13, 2020 14:26
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 @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.

pennylane_qiskit/qiskit_device.py Outdated Show resolved Hide resolved
pennylane_qiskit/qiskit_device.py Outdated Show resolved Hide resolved
pennylane_qiskit/qiskit_device.py Outdated Show resolved Hide resolved
pennylane_qiskit/qiskit_device.py Outdated Show resolved Hide resolved
pennylane_qiskit/qiskit_device.py Outdated Show resolved Hide resolved
tests/test_apply.py Outdated Show resolved Hide resolved
tests/test_apply.py Outdated Show resolved Hide resolved
tests/test_apply.py Outdated Show resolved Hide resolved
tests/test_qiskit_device.py Outdated Show resolved Hide resolved
tests/test_qiskit_device.py Outdated Show resolved Hide resolved
@sagarpahwa
Copy link
Contributor Author

Hi @antalszava , thanks for the thorough review. I made the amendments.

@antalszava antalszava self-requested a review October 15, 2020 18:38
tests/conftest.py Outdated Show resolved Hide resolved
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 @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.

pennylane_qiskit/aer.py Outdated Show resolved Hide resolved
pennylane_qiskit/aer.py Outdated Show resolved Hide resolved
@antalszava
Copy link
Contributor

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 CHANGELOG.md file, you could add an entry to the Improvements section. The convention there is to have a one-sentence description followed by a hyperlink to the PR on the next line (see examples in the changelog). And don't forget to list yourself as a contributor! 😊

@sagarpahwa
Copy link
Contributor Author

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.

Comment on lines +21 to +23
* Qiskit devices are now allowed to pass transpilation options.
[(#108)](https://github.com/PennyLaneAI/pennylane-qiskit/pull/108)

Copy link
Contributor

Choose a reason for hiding this comment

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

💯

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 @sagarpahwa, that's okay, no problem! Sure thing 😊

Looks good to me, think this is good to be merged. Great addition! 🥇 🙂

@josh146 josh146 merged commit 7ba24ac into PennyLaneAI:master Oct 24, 2020
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.

Allow transpilation options to be provided to Qiskit devices
3 participants