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 a backend for IonQ. #391

Merged
merged 14 commits into from
Jun 14, 2021
Merged

Add a backend for IonQ. #391

merged 14 commits into from
Jun 14, 2021

Conversation

amilstead
Copy link
Contributor

Hello ProjectQ!

Here is a first pass at a direct IonQ backend. Welcoming all feedback!

Copy link
Collaborator

@Takishima Takishima left a comment

Choose a reason for hiding this comment

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

Thank you very much for this contribution. I really have not much to say since everything is well-written and documented.

Apart from the two comments that I have made, could you make sure that the year of the license header is 2021 and not 2020?

projectq/backends/_ionq/_ionq_http_client.py Outdated Show resolved Hide resolved
projectq/setups/ionq.py Outdated Show resolved Hide resolved
@amilstead
Copy link
Contributor Author

@Takishima Thanks for the review! I believe this is ready for another pass.

Copy link
Collaborator

@Takishima Takishima left a comment

Choose a reason for hiding this comment

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

Thanks for the latest changes and I am happy to merge the proposed changes.

I am just waiting on the confirmation that you have signed the ProjectQ license agreement (you should have gotten an email or will get it soon about the details of it). Once that is done I will merge the branch and prepare the release of a new version of ProjectQ as soon as possible.

@amilstead
Copy link
Contributor Author

I am just waiting on the confirmation that you have signed the ProjectQ license agreement (you should have gotten an email or will get it soon about the details of it). Once that is done I will merge the branch and prepare the release of a new version of ProjectQ as soon as possible.

Awesome! Thanks a lot! Looking forward to the release. 🎉

@Takishima
Copy link
Collaborator

I have received confirmation that the CLA was signed a few days ago. I will merge this in the coming days and start the release process for the next version of ProjectQ shortly thereafter.
However, I would like to have PR #395 land first (and it should be reviewed soon) since that one has a few important features pertaining to the release process.

@amilstead
Copy link
Contributor Author

I will merge this in the coming days and start the release process for the next version of ProjectQ shortly thereafter.

🎉 Awesome! Thank you!

However, I would like to have PR #395 land first

Sounds good! I'll keep an eye on that PR. Please let me know if there is anything else I can do until the merge.

@amilstead
Copy link
Contributor Author

Hey @Takishima -- any updates on when we might be able to expect #395 to merge?

Thanks!

@Takishima
Copy link
Collaborator

Takishima commented Jun 2, 2021

Hey @Takishima -- any updates on when we might be able to expect #395 to merge?

Thanks!

I wish I could give you a satisfactory answer, but I am currently waiting on @andreashehn to finish his review (although I know he has started looking at it).

Unfortunately, he has been pretty busy with his work over the last few weeks. My hope is that by the end of this week or beginning of the next he should be able to find some time to finish his review and then I will be able to merge #395 and this PR.

@amilstead
Copy link
Contributor Author

Thanks! I'll check back in early next week!

@amilstead
Copy link
Contributor Author

Hey @Takishima,

Checking in as promised. Any luck with #395? I see no movement so far so assuming not, but checking nonetheless.

Copy link
Collaborator

@Takishima Takishima left a comment

Choose a reason for hiding this comment

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

Again, thanks for your contributions and patience with me in this process that is taking way longer than I would like.

One of your latest changes has triggered some warning bells on my side (see comment in the code) and I would be happy to discuss this more. I made one suggestion but if you think you have a better way of solving the issue, I am open to discussing this here.

projectq/backends/_ionq/_ionq.py Outdated Show resolved Hide resolved
@Takishima
Copy link
Collaborator

Hey @Takishima,

Checking in as promised. Any luck with #395? I see no movement so far so assuming not, but checking nonetheless.

No unfortunately not. I am gently reminding him every week without success so far, but I am sure that once things settle down a little for him at work he will submit his review.

@amilstead
Copy link
Contributor Author

@Takishima thanks again for the nudge in the right direction with this qubit mapper. I am WAY happier with how this looks/feels now.

Please let me know what still needs work!

Copy link
Collaborator

@Takishima Takishima left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Takishima
Copy link
Collaborator

So, #395 just landed and it is probably going to require a few modifications to your code, though I do not expect anything too major. You might want to rebase you branch onto the new develop and force-push it to update this PR.

@amilstead
Copy link
Contributor Author

So, #395 just landed and it is probably going to require a few modifications to your code, though I do not expect anything too major. You might want to rebase you branch onto the new develop and force-push it to update this PR.

Thanks! I'll work on this starting today.

amilstead and others added 10 commits June 14, 2021 08:47
Thanks for the correction!

Co-authored-by: Nguyen Damien <ngn.damien@gmail.com>
* Fix some bad logic with result probability mappings.
* Fix some erroneous test cases.
* Fix example code.
* Ensure qubits are "deallocated" and reset after job submit.
* Drop dependency on mappers in favor of API response mappings.
* Add better error handling for API/http errors.
* Fix tests.
* Use a dedicated qubit mapper instead of bashing self.main_engine.
* Update backend, add tests, to reflect new mapper.
@amilstead
Copy link
Contributor Author

@Takishima This should be updated now. That said, I did run into some issues with pytest locally, on revkit code, but this happened on develop for me as well.

One other thing I decided to push here too-- making sure that ProjectQ qubit IDs are actually used when setting Qubit object result values from our API response.

@amilstead amilstead requested a review from Takishima June 14, 2021 14:12
@Takishima
Copy link
Collaborator

I am happy to merge this if you are happy with it too ;-)

@amilstead
Copy link
Contributor Author

I am happy to merge this if you are happy with it too ;-)

Let's rock and roll!! Thanks for your patience and attention on this!

@Takishima
Copy link
Collaborator

I am happy to merge this if you are happy with it too ;-)

Let's rock and roll!! Thanks for your patience and attention on this!

Thank you for taking the time to adjust and tweak your code along the way. I will merge this later tonight and then probably release a new version of ProjectQ by the end of the week.

@amilstead
Copy link
Contributor Author

I am happy to merge this if you are happy with it too ;-)

Let's rock and roll!! Thanks for your patience and attention on this!

Thank you for taking the time to adjust and tweak your code along the way. I will merge this later tonight and then probably release a new version of ProjectQ by the end of the week.

Sounds great. Please let me know what I can do to help, if anything.

@Takishima Takishima merged commit b078f67 into ProjectQ-Framework:develop Jun 14, 2021
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.

None yet

2 participants