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

Remove CPhase gate alias references #718

Merged
merged 20 commits into from
May 30, 2024

Conversation

sergei-mironov
Copy link
Contributor

@sergei-mironov sergei-mironov commented May 6, 2024

Context: Transition to the quantum device config schema 2

Description of the Change:

  • Remove mentions of CPhase "gate" which happens to be a Python alias for ControlledPhaseShift.
  • Remove the Python-only Projector patch because we already have a relaxed validation procedure for observables
  • Relax operation validation by allowing kokkos device to declare C(GlobalPhase) which is a Python-only operation.

Benefits:
Possible Drawbacks:
Related GitHub Issues:

Copy link

codecov bot commented May 6, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.04%. Comparing base (2636257) to head (69a24ab).
Report is 183 commits behind head on main.

Files Patch % Lines
frontend/catalyst/device/qjit_device.py 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #718      +/-   ##
==========================================
- Coverage   98.07%   98.04%   -0.03%     
==========================================
  Files          70       69       -1     
  Lines        9601     9536      -65     
  Branches      756      762       +6     
==========================================
- Hits         9416     9350      -66     
  Misses        151      151              
- Partials       34       35       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented May 6, 2024

Hello. You may have forgotten to update the changelog!
Please edit doc/changelog.md on your branch 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.

@sergei-mironov sergei-mironov changed the title Relax C(GlobalPhase) validation for lightning.kokkos device Relax C(GlobalPhase) and CPhase validation for lightning.kokkos device May 6, 2024
@sergei-mironov sergei-mironov changed the title Relax C(GlobalPhase) and CPhase validation for lightning.kokkos device Relax C(GlobalPhase) and CPhase validation for quantum devices May 6, 2024
@dime10
Copy link
Contributor

dime10 commented May 6, 2024

This can be closed right? My bad I thought this was just about the CPhase gate.

@sergei-mironov
Copy link
Contributor Author

This can be closed right? My bad I thought this was just about the CPhase gate.

Even for CPhase, we need to remove it from tomls contained in the Catalyst repo too.

Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

After #745 is merged and this PR is updated, one should run latest/latest/latest on this branch. I expect it to succeed. On #745 latest/latest/latest fails with the error message:

FAILED frontend/test/pytest/test_transform.py::TestBroadcastExpand::test_expansion_qnode_no_cache[lightning.kokkos-obs1-params2] - catalyst.utils.exceptions.CompileError: Gates in qml.device.operations and specification file do not match.
Gates that present only in the device: {'C(GlobalPhase)'}
Gates that present only in spec: set()

I am not asking for this to fix latest/latest/latest but merely to report what the outcome is on the channel so that we can fix it if this is not the case.

erick-xanadu
erick-xanadu previously approved these changes May 16, 2024
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

Since PR #745 might interfere with this PR I am just going to approve it now. But I will leave it to @dime10 to continue this review. I don't want to block this unnecessarily long. Cheers!

Base automatically changed from toml-schema-2-update-test to main May 24, 2024 14:50
@erick-xanadu erick-xanadu dismissed their stale review May 24, 2024 18:54

I never actually reviewed it, just made the suggestion that it should fix the other issue.

@sergei-mironov
Copy link
Contributor Author

After #745 is merged and this PR is updated, one should run latest/latest/latest on this branch. I expect it to succeed. On #745 latest/latest/latest fails with the error message:

FAILED frontend/test/pytest/test_transform.py::TestBroadcastExpand::test_expansion_qnode_no_cache[lightning.kokkos-obs1-params2] - catalyst.utils.exceptions.CompileError: Gates in qml.device.operations and specification file do not match.
Gates that present only in the device: {'C(GlobalPhase)'}
Gates that present only in spec: set()

I am not asking for this to fix latest/latest/latest but merely to report what the outcome is on the channel so that we can fix it if this is not the case.

@erick-xanadu I merged this PR with the recent main and solved the conflicts. The test you mentioned should pass now.

@sergei-mironov sergei-mironov changed the title Relax C(GlobalPhase) and CPhase validation for quantum devices Remove CPhase gate alias references May 27, 2024
frontend/catalyst/utils/toml.py Outdated Show resolved Hide resolved
frontend/catalyst/utils/toml.py Outdated Show resolved Hide resolved
@sergei-mironov sergei-mironov requested a review from dime10 May 28, 2024 10:45
@sergei-mironov sergei-mironov merged commit 7248c12 into main May 30, 2024
39 of 41 checks passed
@sergei-mironov sergei-mironov deleted the toml-schema-2-relax-c-global-phase branch May 30, 2024 09:25
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.

3 participants