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

Added a warning to ApproxTimeEvolution documentation #5137

Merged
merged 17 commits into from Feb 15, 2024

Conversation

jackyruth
Copy link
Contributor

@jackyruth jackyruth commented Feb 1, 2024

Context:
Two mathematically equal Hamiltonians H1 and H2 pass all the usual equality checks.

c1 = [0.1, 0.2] 
c2 = [0.3, 0.4]
p1 = [qml.PauliZ(0), qml.PauliZ(1)]
p2 = [qml.PauliX(0), qml.PauliX(1)]
H1 = qml.Hamiltonian(c1 + c2, p1 + p2)
H2 = qml.Hamiltonian(c2 + c1, p2 + p1)

But lead to different time evolutions under qml.ApproxTimeEvolution, due to the order in which the observables are stored in qml.Hamiltonian.

dev = qml.device("default.qubit", wires=2)

@qml.qnode(dev)
def circuit(H, t):
    qml.ApproxTimeEvolution(H, t, 1)
    return qml.expval(qml.PauliX(0) @ qml.PauliX(1))

ts = np.linspace(0, 1, 10)
plt.plot(ts, [circuit(H1, t) for t in ts], "x", label="H1")
plt.plot(ts, [circuit(H2, t) for t in ts], "x", label="H2")

This is a tricky scenario that may confuse many users, so it's prudent to add a warning to the qml.ApproxTimeEvolution documentation.

Description of the Change:
Before

After

Benefits:
Save a few hours of debugging for users who are mislead by the "equality" of the hamiltonians.

Possible Drawbacks:
(Slightly) longer qml.ApproxTimeEvolution documentation.

Related GitHub Issues:
#5092

Related Shortcut Stories:
[sc-55193]

Fixes #5092.

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d5e3cca) 99.69% compared to head (3823bf3) 99.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5137      +/-   ##
==========================================
- Coverage   99.69%   99.68%   -0.01%     
==========================================
  Files         394      394              
  Lines       36118    35846     -272     
==========================================
- Hits        36007    35734     -273     
- Misses        111      112       +1     

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

@jackyruth jackyruth changed the title [WIP] Added a warning to ApproxTimeEvolution documentation Added a warning to ApproxTimeEvolution documentation Feb 5, 2024
Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @jackyruth!

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/templates/subroutines/approx_time_evolution.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (249a4bf) 99.68% compared to head (f44d26a) 99.68%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5137      +/-   ##
==========================================
- Coverage   99.68%   99.68%   -0.01%     
==========================================
  Files         396      396              
  Lines       36640    36368     -272     
==========================================
- Hits        36526    36253     -273     
- Misses        114      115       +1     

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

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @jackyruth! Approved, once my comments are resolved.

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
pennylane/templates/subroutines/approx_time_evolution.py Outdated Show resolved Hide resolved
jackyruth and others added 4 commits February 9, 2024 18:08
Co-authored-by: Thomas R. Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Thomas R. Bromley <49409390+trbromley@users.noreply.github.com>
Copy link
Contributor

@Jaybsoni Jaybsoni left a comment

Choose a reason for hiding this comment

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

Hi @jackyruth,

Thanks for the addition! Indeed you are correct, but actually the source of this is a result of the fundamental theory of the Trotter decomposition, which is what is underlying the implementation of ApproxTimeEvolution.

My suggestion would be to rephrase the warning so that it's clear that this divergent behaviour comes from theory and not as a result of the code specifically. Try something like this:

Warning: The Trotter decomposition depends on the order of terms summed together 
in the Hamiltonian. Mathematically identical, but re-ordered, hamiltonians will 
produced different time evolutions in certain parameter regimes. 

Copy link
Contributor

@Jaybsoni Jaybsoni left a comment

Choose a reason for hiding this comment

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

Thanks 💯 !

@trbromley trbromley enabled auto-merge (squash) February 15, 2024 14:02
@trbromley trbromley merged commit 256b1cc into PennyLaneAI:master Feb 15, 2024
36 checks passed
@trbromley
Copy link
Contributor

Thanks @jackyruth!

We often tag contributors in our release marketing on Twitter/X. If you would optionally like to be tagged, please could you share your username?

@jackyruth
Copy link
Contributor Author

jackyruth commented Feb 26, 2024 via email

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.

[BUG] qml.Hamiltonian objects appear to be equivalent, but lead to different qml.ApproxTimeEvolution outcomes
4 participants