Trotterization Demo#1804
Conversation
|
👋 Hey, looks like you've updated some demos! 🐘 Don't forget to update the Please hide this comment once the field(s) are updated. Thanks! |
…adata.json Co-authored-by: Paul Finlay <50180049+doctorperceptron@users.noreply.github.com>
Your preview is ready 🎉!You can view your changes here
|
…into emily/TrotDemo
|
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! |
CatalinaAlbornoz
left a comment
There was a problem hiding this comment.
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
- Stray file named git committed at the repo root (empty, 0 bytes). This looks like an accidental redirect (e.g. ... > git). Please delete it.
- .gitignore change (+1 −1 in demonstrations_v2/.gitignore) — please confirm this is intentional and not a stray edit.
- 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.
- 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
- 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.
- 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.
- Duplicated author in metadata.json for Cirstoiu2020: "Cirstoiu, Holmes, Holmes, Iosue, …" — "Holmes" appears twice.
- Title mismatch: metadata.json title is "Exploring Trotterization", but the demo's H1 is just "Trotterization". Align them.
- [#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.
- [#Suzuki1990]: doi:...
is missing the space afterdoi: that every other reference entry has (doi:...``). - Minor: [#Su2020] is a talk ("presented at the Simons Institute…") but is typed as "article" in metadata.
✍️ Spelling
- Line 44: "eachother" → "each other".
- Line 44: "non-commutivity" → "non-commutativity".
〰️ Hyphenation & consistency
- 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).
- "well known" → "well-known" (ll. 46, 238).
- "tradeoff" (l. 318) vs "trade-offs" (l. 11) — pick one (PennyLane tends to use "trade-off").
- "sublinear" vs "sub-linear" — both appear in the same sentence (l. 320).
- "fault tolerant" → "fault-tolerant" when used as a modifier ("fault-tolerant quantum hardware", "the fault-tolerant picture").
🔤 Grammar
- 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").
- 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
- 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. - 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
- 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.
- 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)
- 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.
- observables (l. 103) is defined but never used.
- 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(...)).
- 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.
…into emily/TrotDemo
|
Hi @CatalinaAlbornoz ! Thanks for the feedback, I have implemented the applicable changes and re-requested review : ) |
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 thenrun
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)