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

Update Permute operation tests #1318

Merged
merged 6 commits into from
May 17, 2021
Merged

Update Permute operation tests #1318

merged 6 commits into from
May 17, 2021

Conversation

antalszava
Copy link
Contributor

@antalszava antalszava commented May 15, 2021

Context

There are test cases for the Permute operation that involves creating a QNode and checking that the underlying tape is expanded. Due to the tape expansion, the Permute operation is decomposed into SWAP operations.

These test cases call the QuantumTape.expand method explicitly. However, as the test cases use a QNode, and default.qubit, the device at hand doesn't support the Permute operation, tape expansion happens internal. Therefore, by the time the second call to the QuantumTape.expand method is placed, the tape has already been expanded.

This second call at the moment leaves the tape as is. However, defining a decomposition for qml.SWAP (as is done in #1317) would result in another depth of tape expansion and in causing errors to the current test cases which assume the tape contains qml.SWAP operations.

Changes

Updates the tests creating a QNode to not call QuantumTape.expand.

@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@codecov
Copy link

codecov bot commented May 15, 2021

Codecov Report

Merging #1318 (f65252d) into master (acec10f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1318   +/-   ##
=======================================
  Coverage   98.16%   98.16%           
=======================================
  Files         148      148           
  Lines       11446    11446           
=======================================
  Hits        11236    11236           
  Misses        210      210           

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 acec10f...f65252d. Read the comment docs.

@antalszava antalszava requested a review from josh146 May 15, 2021 00:50
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Thanks @antalszava for both finding this and fixing this!

Does it make sense for Permute to be a native default.qubit operation in the future? If it is more efficient permuting wires all at once rather than using many swaps, that might potentially this might speed up simulations which use it.

Comment on lines 160 to 161
* Tests for the `Permute` operation are adjusted so that tape expansion happens
once.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not so sure this belongs in 'improvements', as it is a bugfix to the tests suite (and not an improvement in PL directly). Perhaps worth moving to Bug fix, and adding a bit more detail here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good point! Updated

@antalszava
Copy link
Contributor Author

Thank you for the comments @josh146! Created an issue for native support of Permute in default.qubit and updated the changelog.

@antalszava antalszava merged commit b715b35 into master May 17, 2021
@antalszava antalszava deleted the update_permute_tests branch May 17, 2021 14:06
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