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

Fix wires object handling in Sum.terms method #6750

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andrijapau
Copy link
Contributor

@andrijapau andrijapau commented Jan 2, 2025

Context:

Prior to this PR, the wires information for Identity was not being retained correctly,

import pennylane as qml

hamiltonian = qml.dot(
        [1, -0.5, 0.5], [qml.Identity(wires=[0, 1]), qml.PauliZ(0), qml.PauliZ(0)]
)
coeffs, ops = hamiltonian.terms()
I, Z = ops

>>> print(coeffs, ops)
[1.0, np.float64(0.0)] [I(), Z(0)]

>>> print(I.wires, Z.wires)
Wires([]) Wires([0])

Description of the Change:

The origin of the bug is that wire_order in PauliWord.operation was not being updated by the operators current wires set. This is strictly relevant to the Identity operator as it makes use of the argument.

Benefits: Identity.wires are now properly tracked when using Sum.terms.

import pennylane as qml

hamiltonian = qml.dot(
        [1, -0.5, 0.5], [qml.Identity(wires=[0, 1]), qml.PauliZ(0), qml.PauliZ(0)]
)
coeffs, ops = hamiltonian.terms()
I, Z = ops

>>> print(coeffs, ops)
[1.0, np.float64(0.0)] [I(), Z(0)]

>>> print(I.wires, Z.wires)
Wires([0,1]) Wires([0])

Possible Drawbacks: None

[sc-81355]

This comment was marked as resolved.

@andrijapau andrijapau added the WIP 🚧 Work-in-progress label Jan 2, 2025
Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (37abd58) to head (c8d6838).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6750   +/-   ##
=======================================
  Coverage   99.60%   99.60%           
=======================================
  Files         476      476           
  Lines       45210    45210           
=======================================
  Hits        45033    45033           
  Misses        177      177           

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

@andrijapau andrijapau removed the WIP 🚧 Work-in-progress label Jan 2, 2025
@andrijapau andrijapau requested a review from mudit2812 January 2, 2025 19:34
Copy link
Contributor

@JerryChen97 JerryChen97 left a comment

Choose a reason for hiding this comment

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

Can we add a small test that makes use of the wire info of Identity? I feel that without an explicit example people would always wonder 'why do we need the wires of an Identity'.

@andrijapau
Copy link
Contributor Author

andrijapau commented Jan 2, 2025

Hmm, this might have some hidden side-effects that aren't being caught in the test suite. The gradient shape should be of length two here (one per coefficient), but instead I am getting four? 😅

import pennylane as qml

time, n, order = (4.2, 10, 4)

time = qml.numpy.array(time)
hamiltonian = qml.dot(
        [1, 1], [qml.I([0,1]), qml.PauliX(0)]
)

coeffs, ops = hamiltonian.terms()

@qml.qnode(qml.device("default.qubit"), diff_method="backprop")
def circ_bp(coeffs, time):
    with qml.queuing.QueuingManager.stop_recording():
        hamiltonian = qml.dot(coeffs, ops)

    qml.TrotterProduct(hamiltonian, time, n, order)
    return qml.probs()

>>> qml.jacobian(circ_bp)(coeffs, time)
[-0.85459891  0.          0.85459891  0.        ]
# [-0.85459891  0.85459891] on master branch

@andrijapau andrijapau added the do not merge ⚠️ Do not merge the pull request until this label is removed label Jan 2, 2025
@mudit2812
Copy link
Contributor

mudit2812 commented Jan 2, 2025

@andrijapau I actually think that the new behaviour is correct. The hamiltonian you created acts on 2 wires, so the output of qml.probs should have 4 elements.

@JerryChen97
Copy link
Contributor

@andrijapau I actually think that the new behaviour is correct. The hamiltonian you created acts on 2 wires, so the output of qml.probs should have 4 elements.

So the wire info of Identity ops grant us the ability to extend the range of other local ops?

@andrijapau
Copy link
Contributor Author

@andrijapau I actually think that the new behaviour is correct. The hamiltonian you created acts on 2 wires, so the output of qml.probs should have 4 elements.

So the wire info of Identity ops grant us the ability to extend the range of other local ops?

What do you mean by this?

@JerryChen97
Copy link
Contributor

@andrijapau I actually think that the new behaviour is correct. The hamiltonian you created acts on 2 wires, so the output of qml.probs should have 4 elements.

So the wire info of Identity ops grant us the ability to extend the range of other local ops?

What do you mean by this?

Oh nm I'm just thinking that previous results are only showing the non-Identity operators' scope but the new ones reflect a larger scope extended by the Identity, which is actually logically correct here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge ⚠️ Do not merge the pull request until this label is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants