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 usages of ControlledQubitUnitary in tests #1047

Merged
merged 14 commits into from
Feb 3, 2025

Conversation

JerryChen97
Copy link
Contributor

@JerryChen97 JerryChen97 commented Jan 23, 2025

Context:
Due to the deprecations of ControlledQubitUnitary input args PennyLaneAI/pennylane#6839 PennyLaneAI/pennylane#6840, we need to update several tests of lightning that use deprecated interfaces.

Description of the Change:

Benefits:

Possible Drawbacks:

Related GitHub Issues:
[sc-80842]

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.

@JerryChen97 JerryChen97 self-assigned this Jan 23, 2025
@JerryChen97 JerryChen97 added the do not merge Do not merge PR until this label is removed label Jan 23, 2025
@JerryChen97
Copy link
Contributor Author

Do not merge, until the PL deprecation PR's 6839 6840 get merged

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 33.14%. Comparing base (e51170b) to head (8e76831).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (e51170b) and HEAD (8e76831). Click for more details.

HEAD has 93 uploads less than BASE
Flag BASE (e51170b) HEAD (8e76831)
99 6
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1047       +/-   ##
===========================================
- Coverage   97.86%   33.14%   -64.73%     
===========================================
  Files         233       29      -204     
  Lines       39598     2607    -36991     
===========================================
- Hits        38752      864    -37888     
- Misses        846     1743      +897     

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

@JerryChen97
Copy link
Contributor Author

@AmintorDusko Here I investigated the ControlledQubitUnitary occurences that will be affected soon after PL merges the corresponding deprecations. Even though right now I marked it do-not-merge, still want to update with you incase similar urgent hotfix will be needed when the merge happens.

Current test fails are expected.

@JerryChen97 JerryChen97 requested a review from astralcai January 23, 2025 22:17
@AmintorDusko
Copy link
Contributor

Thank you @JerryChen97 for tagging me! We are evaluating the situation. cc: @tomlqc, @maliasadi

@AmintorDusko Here I investigated the ControlledQubitUnitary occurences that will be affected soon after PL merges the corresponding deprecations. Even though right now I marked it do-not-merge, still want to update with you incase similar urgent hotfix will be needed when the merge happens.

Current test fails are expected.

@JerryChen97 JerryChen97 marked this pull request as ready for review January 29, 2025 20:55
@JerryChen97
Copy link
Contributor Author

@AmintorDusko The PennyLane PR to deprecate ControlledQubitUnitary interfaces is very close to merge, shall I ask for a pre-approval for this PR?

@AmintorDusko
Copy link
Contributor

@AmintorDusko The PennyLane PR to deprecate ControlledQubitUnitary interfaces is very close to merge, shall I ask for a pre-approval for this PR?

Could you please check the formatting?

@AmintorDusko
Copy link
Contributor

You may need to update black locally.

@JerryChen97
Copy link
Contributor Author

@JerryChen97
Copy link
Contributor Author

make format works, thanks to @josephleekl

@AmintorDusko
Copy link
Contributor

@AmintorDusko The PennyLane PR to deprecate ControlledQubitUnitary interfaces is very close to merge, shall I ask for a pre-approval for this PR?

@JerryChen97, I prefer if we can have PennyLane PR merged first and approve this PR when CIs are fully green. I hope you don't mind. If there is any special urgency for that let me know.

@JerryChen97
Copy link
Contributor Author

@AmintorDusko The PennyLane PR to deprecate ControlledQubitUnitary interfaces is very close to merge, shall I ask for a pre-approval for this PR?

@JerryChen97, I prefer if we can have PennyLane PR merged first and approve this PR when CIs are fully green. I hope you don't mind. If there is any special urgency for that let me know.

Sure! I'll also tag Alex to update the decision here @Alex-Preciado

JerryChen97 added a commit to PennyLaneAI/pennylane that referenced this pull request Jan 31, 2025
**Context:**
We are in favor of `wires`, just like what we did with
`MultiControlledX`.

!Note: this involves a big user-interface change. With PL version
<=0.40, for ControlledQubitUnitary, we have arg wires to be the
target_wires, and the arg control_wires is a positional arg. To be
consistent with all the other interfaces, we would like to alter this
into wires to be positional and to represent control_wires+target_wires

Before:
```python
mat = np.eye(2)
control_wires = [0]
wires = [1]
ControlledQubitUnitary(mat, control_wires, wires=wires, control_values=...)
```

After:
```python
mat = np.eye(2)
wires = [0, 1]
ControlledQubitUnitary(mat, wires, control_values=...)
```

where the new `wires` should be the concatenate of old `control_wires`
and `wires`

**Description of the Change:**
Besides the modified test files across the whole codebase involving
`ControlledQubitUnitary`, we made the following necessary adjustments
beyond the `__init__` method:
- classmethod `_unflatten`, `_primitive_bind_call` and method
`_controlled` were adjusted accordingly following the new interface
 - move the `TypeError` regarding empty arg `wires` to the beginning.
- add a register for `ControlledQubitUnitary` in
`bind_new_parameters.py` following the pattern of `CRX`
- modified the direct calls of `ControlledQubitUnitary` inside src files
`metric_tensor.py`, `controlled.py` and `matrix_ops.py`.

### **Eco-System**
This is an inevitable breaking change, so we need to immediately inform
the affected repos that depend on latest PennyLane but only apply
corresponding changes after this PR gets merged.

- [ ] labs

- [ ] Catalyst:
[frontend/test/pytest/device/test_decomposition.py](https://github.com/PennyLaneAI/catalyst/blob/286e51910a39c55130de8c1c6a806818a2bcb14b/frontend/test/pytest/device/test_decomposition.py#L52)
PennyLaneAI/catalyst#1483

- [ ] Lightning: PR to fix it
PennyLaneAI/pennylane-lightning#1047
- -
[test_gates.py](https://github.com/PennyLaneAI/pennylane-lightning/blob/290c986e7d087e17dbf3f09c6d2dfc8bd738cc08/tests/test_gates.py#L451)
- -
[test_gates_and_expval.py](https://github.com/PennyLaneAI/pennylane-lightning/blob/290c986e7d087e17dbf3f09c6d2dfc8bd738cc08/tests/lightning_tensor/test_gates_and_expval.py#L72)
- -
[test_measurements_class.py](https://github.com/PennyLaneAI/pennylane-lightning/blob/290c986e7d087e17dbf3f09c6d2dfc8bd738cc08/tests/lightning_qubit/test_measurements_class.py#L814)

- [ ] QML Demos:
[tutorial_classically_boosted_vqe.py](https://github.com/PennyLaneAI/qml/blob/c444cec6201338056304d9d5a293f969a3aeec5e/demonstrations/tutorial_classically_boosted_vqe.py#L451);
PR to fix it PennyLaneAI/qml#1310

### **Plugins**
- [x] Pennylane-AQT: No instances of deprecated code found.
- [x] Pennylane-Qiskit: No instances of deprecated code found.
- [x] Pennylane-IonQ: No instances of deprecated code found.
- [x] Pennylane-Qrack: No instances of deprecated code found.
- [x] Pennylane-Cirq: No instances of deprecated code found.
- [x] Pennylane-Qulacs: No instances of deprecated code found.

**Benefits:**
Unified interfaces with other controlled ops.

**Possible Drawbacks:**
This is a hard change to deprecate this arg. Would have to introduce
errors for testing matrices.

**Related GitHub Issues:**
[sc-80842]

---------

Co-authored-by: Christina Lee <[email protected]>
Co-authored-by: Astral Cai <[email protected]>
@JerryChen97 JerryChen97 removed the do not merge Do not merge PR until this label is removed label Jan 31, 2025
@JerryChen97
Copy link
Contributor Author

@AmintorDusko The PennyLane PR to deprecate ControlledQubitUnitary interfaces is very close to merge, shall I ask for a pre-approval for this PR?

@JerryChen97, I prefer if we can have PennyLane PR merged first and approve this PR when CIs are fully green. I hope you don't mind. If there is any special urgency for that let me know.

Sure! I'll also tag Alex to update the decision here @Alex-Preciado

PL PR merged.

@JerryChen97
Copy link
Contributor Author

all checks passed

@JerryChen97
Copy link
Contributor Author

@AmintorDusko

Copy link
Contributor

@AmintorDusko AmintorDusko 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.

@JerryChen97 JerryChen97 merged commit 2ef808a into master Feb 3, 2025
69 of 70 checks passed
@JerryChen97 JerryChen97 deleted the fix-ControlledQubitUnitary-usage branch February 3, 2025 20:23
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.

5 participants