Skip to content

Trotterization Demo#1804

Open
nobese wants to merge 67 commits into
masterfrom
emily/TrotDemo
Open

Trotterization Demo#1804
nobese wants to merge 67 commits into
masterfrom
emily/TrotDemo

Conversation

@nobese

@nobese nobese commented May 29, 2026

Copy link
Copy Markdown
Contributor

Before submitting

Please complete the following checklist when submitting a PR:

  • Ensure that your tutorial executes correctly, and conforms to the
    guidelines specified in the README.

  • Remember to do a grammar check of the content you include.

  • All tutorials conform to
    PEP8 standards.
    To auto format files, simply pip install black, and then
    run black -l 100 path/to/file.py.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Title:

Summary:

Relevant references:

Possible Drawbacks:

Related GitHub Issues:


If you are writing a demonstration, please answer these questions to facilitate the marketing process.

  • GOALS — Why are we working on this now?

    Eg. Promote a new PL feature or show a PL implementation of a recent paper.

  • AUDIENCE — Who is this for?

    Eg. Chemistry researchers, PL educators, beginners in quantum computing.

  • KEYWORDS — What words should be included in the marketing post?

  • Which of the following types of documentation is most similar to your file?
    (more details here)

  • Tutorial
  • Demo
  • How-to

@nobese
nobese requested review from a team as code owners May 29, 2026 15:20
@github-actions

Copy link
Copy Markdown

👋 Hey, looks like you've updated some demos!

🐘 Don't forget to update the dateOfLastModification in the associated metadata files so your changes are reflected in Glass Onion (search and recommendations).

Please hide this comment once the field(s) are updated. Thanks!

…adata.json

Co-authored-by: Paul Finlay <50180049+doctorperceptron@users.noreply.github.com>
@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

Your preview is ready 🎉!

You can view your changes here

Deployed at: 2026-07-24 14:12:47 UTC

@nobese

nobese commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the comments @josh146 ! Sorry for the delay in addressing them, I missed the notification. I believe that I hit all of your points, particularly reworking the explanation of why increased complexity makes it harder to implement the time evolution unitary. Let me know if you have any more comments!

@nobese
nobese requested a review from josh146 July 10, 2026 20:37

@josh146 josh146 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work @nobese! Happy to approve, just a few typos and links to correct before merging.

Comment thread demonstrations_v2/exploring_trotterization/demo.py Outdated
Comment thread demonstrations_v2/exploring_trotterization/demo.py Outdated
Comment thread demonstrations_v2/exploring_trotterization/demo.py Outdated
Comment thread demonstrations_v2/exploring_trotterization/demo.py Outdated
Comment thread demonstrations_v2/exploring_trotterization/demo.py Outdated
Comment thread demonstrations_v2/exploring_trotterization/demo.py Outdated
Comment thread demonstrations_v2/exploring_trotterization/demo.py Outdated
@CatalinaAlbornoz
CatalinaAlbornoz self-requested a review July 21, 2026 20:38

@CatalinaAlbornoz CatalinaAlbornoz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @nobese, I used AI to review the demo and it noticed a couple major issues (and a few minor ones) that need to be resolved before we can merge.


🔴 Blocking / high-priority

  1. Stray file named git committed at the repo root (empty, 0 bytes). This looks like an accidental redirect (e.g. ... > git). Please delete it.
  2. .gitignore change (+1 −1 in demonstrations_v2/.gitignore) — please confirm this is intentional and not a stray edit.
  3. PEP8 / black non-conformance (the PR checklist claims conformance). black -l 100 --check fails. Concretely:
  • Missing spaces around operators: 2*coeffs[0]*del_t, t/r, etc.
  • Spaces around = in keyword args: wires = 0, n = num_trotter_steps, order = 1 should be wires=0, etc.
  • Comments without a space after # (E265): 11 instances (#Define System Parameters, #Apply the rotation r times, …).
  • ~20 lines with trailing whitespace in the docstring.
  • Please run black -l 100 demo.py.
  1. CamelCase function names violate PEP8 (functions should be snake_case): TrotterStepper, BuiltInTrotter, TrotterStepperSO, TrotterStepperPG → e.g. trotter_stepper, built_in_trotter, trotter_stepper_so, trotter_stepper_pg.

📚 References & metadata

  1. Su2020 is missing from metadata.json. It's cited inline ([#Su2020]_) and appears in the RST reference list, but not in the metadata references array.
  2. Two orphan references — Childs2010 ("Limitations on the simulation of non-sparse Hamiltonians") and Paetznick2014 ("Repeat-Until-Success…") are defined in the RST reference list and in metadata but are never cited inline, and their topics don't match this demo. These look like leftover template references — uncited targets also produce Sphinx warnings. Please remove them or cite them.
  3. Duplicated author in metadata.json for Cirstoiu2020: "Cirstoiu, Holmes, Holmes, Iosue, …" — "Holmes" appears twice.
  4. Title mismatch: metadata.json title is "Exploring Trotterization", but the demo's H1 is just "Trotterization". Align them.
  5. [#Verlet1967] title has unescaped nested quotes in demo.py: "Computer "Experiments" on Classical Fluids…" — this will render awkwardly. The metadata version uses single quotes ('Experiments'); mirror that or escape the inner quotes.
  6. [#Suzuki1990]: doi:...is missing the space afterdoi: that every other reference entry has (doi: ...``).
  7. Minor: [#Su2020] is a talk ("presented at the Simons Institute…") but is typed as "article" in metadata.

✍️ Spelling

  1. Line 44: "eachother" → "each other".
  2. Line 44: "non-commutivity" → "non-commutativity".

〰️ Hyphenation & consistency

  1. Inconsistent ordinal-modifier hyphenation. Compound modifiers before a noun should be hyphenated. Currently mixed: "first**-**order" is used in most places but not in "first order Trotterization expression" (l. 51), "first order term" (l. 192), or "higher order terms" (l. 192).
  2. "well known" → "well-known" (ll. 46, 238).
  3. "tradeoff" (l. 318) vs "trade-offs" (l. 11) — pick one (PennyLane tends to use "trade-off").
  4. "sublinear" vs "sub-linear" — both appear in the same sentence (l. 320).
  5. "fault tolerant" → "fault-tolerant" when used as a modifier ("fault-tolerant quantum hardware", "the fault-tolerant picture").

🔤 Grammar

  1. Line 243: "allowing for resources to be allocated to only the instances they are justified" — missing a word; → "…to only the instances where they are justified" (or "in which they are justified").
  2. Line 320: "requires the Hamiltonian have a known analytical solution" — reads better as "requires that the Hamiltonian have a known analytical solution."

➗ Math rigor / presentation

  1. Second-order formula (≈ l. 197):
    e^{-iαXt}e^{-iβZt} = (e^{-iαXt/2r} e^{-iβZt/r} e^{-iαXt/2r})
    The left side is the full first-order product over time t, while the right side is a single per-step term (note the /r and /2r) — these aren't equal, and there's no outer ^r. Consider writing it as the per-step symmetric operator S_2(Δt) = e^{-iαXΔt/2} e^{-iβZΔt} e^{-iαXΔt/2} with ≈ and the (...)^r for the full evolution.
  2. Several exact-looking = signs are really approximations, e.g. l. 51 e^{-iHt}=(∏_j e^{-iH_j t/r})^r and l. 88 U(t)=e^{-iβZt}e^{-iαXt}=…. Since the whole point of the demo is that Trotterization is approximate, consider ≈ where appropriate.

🎨 Style vs other PennyLane demos

  1. Section headings use Title Case ("The Commutation Problem", "Implementing the Trotter Method", "Gate Synthesis Considerations", "Higher-Order Trotterizations"). PennyLane's style guide generally prefers sentence case for headings (e.g. "The commutation problem"). Worth aligning.
  2. Tone/exclamation density. The voice is charming, but the exclamations and asides pile up ("Rats!", "Not too shabby!", "Ah ha!", "Keep calm and Trotter on!", "time is of the essence!", "This becomes unaffordable very quickly!"). PennyLane demos are friendly but a bit more even — consider trimming a few so the strongest ones land.

🧹 Code cleanups (non-blocking)

  1. first_order_expansion(time, theta, phi, wires) ignores its wires argument and hardcodes wires=0. Since trotterize passes wires in, use the parameter (wires=wires[0]) for correctness and clarity.
  2. observables (l. 103) is defined but never used.
  3. TrotterStepperSO has dead assignments: U_A_half = qp.RX(...) (reassigned) and U_B = qp.RZ(...) are never used; the gate calls have side effects, so drop the assignments. Same pattern in TrotterStepperPG (U_B = qp.RZ(...)).
  4. Loop counters for i in range(r) don't use i — use for _ in range(r).

Summary

The demo runs correctly on v0.45.1 and is scientifically sound — no functional blockers. Before merge I'd prioritize: (1) removing the stray git file, (2) black/PEP8 + snake_case cleanup, and (3) the reference/metadata reconciliation (missing Su2020, orphan Childs2010/Paetznick2014, duplicated "Holmes", title mismatch). The spelling, hyphenation, and math-presentation items are quick polish passes on top.

@nobese

nobese commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Hi @CatalinaAlbornoz ! Thanks for the feedback, I have implemented the applicable changes and re-requested review : )

@nobese
nobese requested a review from CatalinaAlbornoz July 22, 2026 14:47
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