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.

Sign up for GitHub

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

Jump to bottom

Fix extra CNOT inserted in Patel-Hayes-Markov algorithm #5044

Merged
merged 12 commits into from
Sep 11, 2020
Merged

Fix extra CNOT inserted in Patel-Hayes-Markov algorithm #5044

merged 12 commits into from
Sep 11, 2020

Conversation

meamy
Copy link
Contributor

@meamy meamy commented Sep 8, 2020 •
edited by 1ucian0
Loading

Summary

Fixes #2718

Details and comments

Happened to see this and thought I would fix it quickly. The graysynth part had a bug where the output of Patel-Markov-Hayes was reversed. The extra CNOT is just what the PMH algorithm on its own produces, but I had a tweak in my implementation that dealt with these situations so I added it in here.

@meamy meamy requested a review from a team as a code owner September 8, 2020 15:32
@CLAassistant
Copy link

CLAassistant commented Sep 8, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@1ucian0 1ucian0 self-assigned this Sep 10, 2020
1ucian0
1ucian0 requested changes Sep 10, 2020
View reviewed changes
Copy link
Member

@1ucian0 1ucian0 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot for this fix! It's always nice to see an old issue being closed :)

Some comments inlined. I would also add a test for example 4.3

@1ucian0 1ucian0 changed the title Fix for issue #2718 (extra CNOT inserted in Patel-Hayes-Markov algorithm) Fix extra CNOT inserted in Patel-Hayes-Markov algorithm Sep 11, 2020
@1ucian0
Copy link
Member

1ucian0 commented Sep 11, 2020

I would also add a test for example 4.3

What do you think about this?

@meamy
Copy link
Contributor Author

meamy commented Sep 11, 2020

Sure, I added example 4.3 as a test case which exposed a bug with PMH

1ucian0
1ucian0 approved these changes Sep 11, 2020
View reviewed changes
Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Thanks a lot ! Welcome to the project!

@mergify mergify bot merged commit 7bc7f08 into Qiskit:master Sep 11, 2020
@meamy
Copy link
Contributor Author

meamy commented Sep 14, 2020

No problem! Happy to help :)

@meamy meamy deleted the graysynth-fix branch September 14, 2020 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1ucian0 1ucian0 1ucian0 approved these changes

Assignees

@1ucian0 1ucian0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

extra CNOT inserted in Patel-Hayes-Markov algorithm
3 participants
@meamy @CLAassistant @1ucian0