Fix double QubitUnitary bug from Catalyst

[JerryChen97]

Context:
A recent deprecation #6840 deprecated the usage of QubitUnitary type input base for instantiation of ControlledQubitUnitary.
However, a modification that was equivalent in PennyLane caused unexpected behavior in Catalyst, PennyLaneAI/catalyst#1494.
After investigation, it appears to be the issue with the private method _try_wrap_in_custom_ctrl_op of pennylane/ops/op_math/controlled.py, since it used to call the deprecated init to directly use base as QubitUnitary, and to safely deprecate it we decided to locally replace with equivalent ControlledQubitUnitary(base=op.matrix(), ...), this caused Catalyst to received double QubitUnitary.

It is also noteable that the call ControlledQubitUnitary should not appear in this script, since logically all the classes living in controlled_ops depend on controlled.py, which means that from the very beginning the affected private method served as a tricky hack with cyclic dependence.

Therefore, we would like to replace the ControlledQubitUnitary call with a direct ControlledOp.

Description of the Change:
source files: as mentioned above.

tests: necessary improvements were made to correctly reflect the assertion results.

Benefits:

• Remove bugs
• Less cyclic dependency

Possible Drawbacks:

• Probably there're more that depend on this method, which might be affected by this fix

Related GitHub Issues:
PennyLaneAI/catalyst#1494

[JerryChen97]

@erick-xanadu we can try to test the affected tmrw.
On my end it looks fixed

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.59%. Comparing base (9744822) to head (9e06caa).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6926   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files         480      480           
  Lines       45495    45496    +1     
=======================================
+ Hits        45310    45311    +1     
  Misses        185      185           

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

[erick-xanadu]

@JerryChen97, yes I also tested it and it works! Thanks! I'm just running Catalyst tests to see if anything else breaks but I don't think so. I would also like to spend a little bit of time see if we can fix this in Catalyst. But it is good to know that there is a fix that solves it.

[erick-xanadu]

Thanks @JerryChen97 , I left an alternative solution in the comments. Both solutions work. I think mine works because the previous operation was removed from the queue. I'm not entirely sure yet why your approach works. Do you know?

[andrijapau]

Makes sense to me! 🚀

[JerryChen97]

Thanks @JerryChen97 , I left an alternative solution in the comments. Both solutions work. I think mine works because the previous operation was removed from the queue. I'm not entirely sure yet why your approach works. Do you know?

My approach was only considering the behavior diff between legacy ControlledQubitUnitary (abbr as CQU later) and the new one.

The redundant QubitUnitary (QU) comes from that to deprecate the CQU interface of taking a QU as base, I had to convert all CQU(base=QU, ...) into CQU(base=QU.matrix(), ...). But due to the very original design, CQU init eventually has to call ControlledOp(base=QU, ...), so there an extra instance of QU was introduced...... To avoid such redundancy, skipping the new init of CQU is good enough.

However, I do have concerns about such hacky fix. Especially what if the other methods implemented in real CQU are also important for Catalyst? If this happens I'll say your approach is better since at least having a CQU here is just logically not good but has been working all the way until recent deprecatioin. We might want to ask @albi3ro to join and see if anything more needs to be considered.

On my end I would like to also try moving CQU to controlled.py to see if it's equivalent to just have it altoghether with ctrl and ControlledOp. If so then it's totally fine to just keep CQU there and to remove the first QU from queue, despite still cyclic references exist.

[JerryChen97]

Do not merge until concerns were cleared

[albi3ro]

Can we just add it to ops_with_custom_ctrl_op and remove the special QubitUnitary logic? Looks to like it it should be able to fit that mold now.

[albi3ro]

Requesting changes based on updating the name.

[albi3ro]

Can we just add it to ops_with_custom_ctrl_op and remove the special QubitUnitary logic? Looks to like it it should be able to fit that mold now.

Never mind, I just realized ControlledQubitUnitary allows any number of control wires, so it does not fit the mold.

[JerryChen97]

Added full test suite to see if we broke any other pipelines

[rashidnhm]

Hello, from a quick pull request filter, it seems this is currently the only PR that needs gpu test suite run on it.

I have changed the gpu test suite as part of #6927 . Please pull in latest master to ensure the gpu tests continue to be run.

I will be disabling the existing gpu runners shortly.

[albi3ro]

Can we also add a test to make sure the bug is really fixed? Maybe something like:

with qml.queuing.AnnotatedQueue() as q:
    qml.ctrl(qml.QubitUnitary(np.eye(2), 0), 1)

assert len(q.queue) == 1

[JerryChen97]

Hello, from a quick pull request filter, it seems this is currently the only PR that needs gpu test suite run on it.

I have changed the gpu test suite as part of #6927 . Please pull in latest master to ensure the gpu tests continue to be run.

I will be disabling the existing gpu runners shortly.

No wry! After confirming nothing else broken we don't need GPU tests any more. I'll move labels

[JerryChen97]

Can we also add a test to make sure the bug is really fixed? Maybe something like:

with qml.queuing.AnnotatedQueue() as q:
    qml.ctrl(qml.QubitUnitary(np.eye(2), 0), 1)

assert len(q.queue) == 1

@albi3ro Done!

[albi3ro]

Left a comment on the changelog entry, but looks good 👍