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

Add IonQ native gateset support #55

Merged
merged 6 commits into from
May 16, 2022
Merged

Conversation

Cynocracy
Copy link
Contributor

No description provided.

@Cynocracy
Copy link
Contributor Author

Will have to go to an older black version for formatting, I guess.

The TODOs need to be resolved before submission, and the too-few-public-method check should be skipped/ignored somehow

Can add tests once we finalize API shape

@josh146
Copy link
Member

josh146 commented May 5, 2022

Thanks @Cynocracy!

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #55 (4fba456) into master (7580077) will increase coverage by 4.92%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   88.85%   93.77%   +4.92%     
==========================================
  Files           5        5              
  Lines         287      305      +18     
==========================================
+ Hits          255      286      +31     
+ Misses         32       19      -13     
Impacted Files Coverage Δ
pennylane_ionq/__init__.py 100.00% <100.00%> (ø)
pennylane_ionq/_version.py 100.00% <100.00%> (ø)
pennylane_ionq/device.py 92.98% <100.00%> (-0.42%) ⬇️
pennylane_ionq/ops.py 100.00% <100.00%> (+100.00%) ⬆️
pennylane_ionq/api_client.py 93.08% <0.00%> (-1.89%) ⬇️

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 62277c3...4fba456. Read the comment docs.

@Cynocracy
Copy link
Contributor Author

Ready for review!

@josh146 josh146 requested a review from albi3ro May 11, 2022 21:02
pennylane_ionq/ops.py Outdated Show resolved Hide resolved
@Cynocracy Cynocracy requested a review from albi3ro May 12, 2022 18:00
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

I was able to submit a job to the simulator with the native gate set, but "ionq.qpu" is hanging currently for both "native" and "qis" gates.

Exciting to be able to work with these gates!

Most of my notes have to do with documentation.

We could probably add parameter-shift support later for these gates, but I think it's ok to leave them with grad_method = None for now.

"""
num_params = 1
num_wires = 1
grad_method = "None"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
grad_method = "None"
grad_method = None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an example of how to add parameter shift support? I'd be happy to give it a shot (though maybe in a follow-up PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

The easiest way is to define the parameter_frequencies property.

If the parameter frequencies aren't manually defined, we can calculate them from the generator, defined via the generator method.

As these are single qubit operators with a single parameter, they will probably have the same frequencies as the standard rotation gates, namely [(1,)], but I'd have to double check.

pennylane_ionq/ops.py Outdated Show resolved Hide resolved
pennylane_ionq/ops.py Outdated Show resolved Hide resolved
pennylane_ionq/ops.py Outdated Show resolved Hide resolved
pennylane_ionq/ops.py Outdated Show resolved Hide resolved
pennylane_ionq/ops.py Show resolved Hide resolved
pennylane_ionq/__init__.py Outdated Show resolved Hide resolved
pennylane_ionq/device.py Show resolved Hide resolved
@Cynocracy
Copy link
Contributor Author

Is the QPU job submitting fine but then not being processed? We have very long queue times at the moment: https://status.ionq.co/

@Cynocracy
Copy link
Contributor Author

Okay, after many mini fixups I think the docs render and don't have any glaring typos (I hope!)

@Cynocracy Cynocracy requested a review from albi3ro May 13, 2022 20:43
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

The .. math:: accepts LaTeX, so there's no need for the * symbols.

Thanks for updating the docstring for the IonQDevice class. We should similarly update the docstring for the QPUDevice and the SimulatorDevice.

pennylane_ionq/ops.py Outdated Show resolved Hide resolved
pennylane_ionq/ops.py Outdated Show resolved Hide resolved
pennylane_ionq/ops.py Outdated Show resolved Hide resolved
@Cynocracy
Copy link
Contributor Author

docs look good!

@Cynocracy Cynocracy requested a review from albi3ro May 16, 2022 19:24
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for being patient will all the changes.

@albi3ro albi3ro merged commit 2fd22c9 into PennyLaneAI:master May 16, 2022
@Cynocracy Cynocracy deleted the native-gates branch May 16, 2022 21:54
@Cynocracy
Copy link
Contributor Author

No problem :) I appreciate the thorough review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants