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

Use native implementation for adjoints in (control) operations #1063

Merged
merged 32 commits into from
Feb 26, 2025

Conversation

josephleekl
Copy link
Contributor

@josephleekl josephleekl commented Feb 11, 2025

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    tests directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the .github/CHANGELOG.md file, summarizing the
    change, and including a link back to the PR.

  • Ensure that code is properly formatted by running make format.

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


Context:
Currently in _apply_lightning, we check for whether an operation is Adjoint, then we apply the operation with an adjoint (inv_param) flag. However, in cases where we have:

  • adjoint(s) within control - e.g. control(adjoint(gate))
  • control within adjoint - e.g. adjoint(control(gate)),

these are all applied as matrices.

Description of the Change:
_apply_lightning and _apply_lightning_controlled checks for adjoint in an operation, and if it's an adjoint it applies the base operation with an adjoint flag, instead of treating everything as a matrix.

So in effect we have:
control(adjoint(gate)) -> control(gate with adjoint)
adjoint(control(gate)) -> control(gate with adjoint)

which are implemented natively in C++ (if the gate is supported), yielding better performance

Benefits:
adjoint(ctrl()) will see the most speedup, especially with large number of control wires, since we use native control operation which contains less wires than the equivalent matrix, and needs to be operated on less wires. adjoint(ctrl()) will see some speed-up, since we are now able to use the native named gate implementation in C++.

Example timing improvement:
4 ctrl wires

LQ:

LQ, 25 qubits, 500 repeats master branch
ctrl(adjoint(IsingXX)) 9.6s 6.0s
ctrl(adjoint(DoubleExcitationPlus)) 27.6s 9.2s
LQ, 25 qubits, 100 repeats master branch
adjoint(ctrl(IsingXX)) 267s 2.9s
adjoint(ctrl(DoubleExcitationPlus)) 1002s 3.6s

Baseline:

LQ, 25 qubits, 500 repeats master branch
ctrl(IsingXX) 6.1s 6.1s
ctrl(DoubleExcitationPlus) 9.1s 9.1s

LG:

LG, 31 qubits, 1000 repeats master branch
ctrl(adjoint(IsingXX)) 4.9s 4.8s
ctrl(adjoint(DoubleExcitationPlus)) 5.0s 4.9s
LG, 31 qubits, 1000 repeats master branch
adjoint(ctrl(IsingXX)) 119s 4.8s
adjoint(ctrl(DoubleExcitationPlus)) 208s 4.9s

Baseline:

LG, 31 qubits, 1000 repeats master branch
ctrl(IsingXX) 4.8s 4.8s
ctrl(DoubleExcitationPlus) 4.9s 4.9s

LK:

LK, 25 qubits, 500 repeats master branch
ctrl(adjoint(IsingXX)) 8.5s 5.7s
ctrl(adjoint(DoubleExcitationPlus)) 24.5s 7.6s
LK, 25 qubits, 100 repeats master branch
adjoint(ctrl(IsingXX)) 235s 2.6s
adjoint(ctrl(DoubleExcitationPlus)) 867s 2.9s

Baseline:

LK, 25 qubits, 500 repeats master branch
ctrl(IsingXX) 5.6s 5.8s
ctrl(DoubleExcitationPlus) 7.7s 7.6 s

Possible Drawbacks:

Related GitHub Issues:

[sc-79430]

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 85.07463% with 10 lines in your changes missing coverage. Please review.

Project coverage is 97.35%. Comparing base (aad3e59) to head (a582803).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pennylane_lightning/lightning_gpu/_state_vector.py 9.09% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
+ Coverage   96.10%   97.35%   +1.25%     
==========================================
  Files         232      111     -121     
  Lines       39178    16997   -22181     
==========================================
- Hits        37651    16547   -21104     
+ Misses       1527      450    -1077     

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

@josephleekl
Copy link
Contributor Author

josephleekl commented Feb 11, 2025

Only LQ is implemented so far - but if this is OK, I'll propagate it to LK and LG, which is fairly straightforward
update: LK and LG added

@josephleekl josephleekl marked this pull request as ready for review February 11, 2025 19:30
@josephleekl josephleekl changed the title Recursively eliminate adjoints in (control) operations Use native implementation for adjoints in (control) operations Feb 12, 2025
@josephleekl josephleekl added the ci:use-gpu-runner Enable usage of GPU runner for this Pull Request label Feb 12, 2025
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Thanks @josephleekl!💡 Just a few comments before pushing it to master

Copy link
Contributor

@LuisAlfredoNu LuisAlfredoNu left a comment

Choose a reason for hiding this comment

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

Thanks @josephleekl
Just a few comments ❄️

josephleekl and others added 4 commits February 24, 2025 19:54
**Context:**

In PennyLaneAI/pennylane#6991 we are adding an
`execution_config` kwarg to `Device.eval_jaxpr`. We need to make sure
this change doesn't break lightning.

**Description of the Change:**

Adds an `execution_config : Optional[ExecutionConfig] = None` keyword
argument to `Device.eval_jaxpr`.

**Benefits:**

Lightning won't break when that change gets merged into pennylane. The
lightning device jaxpr execution can be configured in the future.

**Possible Drawbacks:**

**Related GitHub Issues:**

[sc-84916]

---------

Co-authored-by: ringo-but-quantum <github-ringo-but-quantum@xanadu.ai>
Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>
@josephleekl josephleekl removed the ci:use-gpu-runner Enable usage of GPU runner for this Pull Request label Feb 25, 2025
@josephleekl josephleekl added the ci:use-gpu-runner Enable usage of GPU runner for this Pull Request label Feb 25, 2025
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Nice work @josephleekl! Happy to approve 🎉

Copy link
Contributor

@LuisAlfredoNu LuisAlfredoNu left a comment

Choose a reason for hiding this comment

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

Thank you @josephleekl

Happy to approve 🪂

@josephleekl josephleekl merged commit 98a9292 into master Feb 26, 2025
72 of 73 checks passed
@josephleekl josephleekl deleted the recursive-ctrl-adj branch February 26, 2025 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:use-gpu-runner Enable usage of GPU runner for this Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants