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

Add functions to support a sparse matrix observable #1398

Merged
merged 53 commits into from
Jun 29, 2021
Merged

Conversation

soranjh
Copy link
Contributor

@soranjh soranjh commented Jun 9, 2021

Context:

This PR adds a new observable class for supporting sparse matrix objects and adds the functionality to compute the expectation value of the observable via direct matrix-vector multiplication.

Description of the Change:

The observable class SparseHamiltonian is added to the qml.ops.qubit module and the new function expval is created in qml.devices.default_qubit.

Benefits:

The computation of a Hamiltonian expectation value is faster and more memory-efficient with the new functionality.

@soranjh soranjh added the WIP 🚧 Work-in-progress label Jun 9, 2021
@soranjh soranjh self-assigned this Jun 9, 2021
@soranjh soranjh requested a review from ixfoduap June 14, 2021 14:50
@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #1398 (c97865d) into master (614db57) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1398   +/-   ##
=======================================
  Coverage   98.23%   98.24%           
=======================================
  Files         160      160           
  Lines       11966    11991   +25     
=======================================
+ Hits        11755    11780   +25     
  Misses        211      211           
Impacted Files Coverage Δ
pennylane/devices/default_qubit.py 100.00% <100.00%> (ø)
pennylane/ops/qubit.py 98.73% <100.00%> (+0.01%) ⬆️
pennylane/qnode.py 98.01% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 614db57...c97865d. Read the comment docs.

Copy link
Contributor

@ixfoduap ixfoduap left a comment

Choose a reason for hiding this comment

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

Looks really good! It's crazy that some much work went into understanding how to speed up simulations, and in the end the change is so simple.

My main suggestions are to improve the documentation and to make the tests more compact when specifying the Hamiltonians

.github/CHANGELOG.md Outdated Show resolved Hide resolved
pennylane/devices/default_qubit.py Outdated Show resolved Hide resolved
pennylane/devices/default_qubit.py Show resolved Hide resolved
pennylane/devices/default_qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
tests/devices/test_default_qubit.py Outdated Show resolved Hide resolved
tests/devices/test_default_qubit.py Outdated Show resolved Hide resolved
@soranjh soranjh added merge-ready ✔️ All tests pass and the PR is ready to be merged. and removed WIP 🚧 Work-in-progress labels Jun 15, 2021
@soranjh soranjh changed the title [WIP] add functions to support a sparse matrix observable Add functions to support a sparse matrix observable Jun 15, 2021
@soranjh soranjh marked this pull request as ready for review June 15, 2021 21:51
@soranjh soranjh removed the merge-ready ✔️ All tests pass and the PR is ready to be merged. label Jun 16, 2021
Co-authored-by: ixfoduap <40441298+ixfoduap@users.noreply.github.com>
@soranjh soranjh requested review from josh146 and ixfoduap June 22, 2021 20:17
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Thanks @soranjh! Main comment here is mostly about having qml.SparseHamiltonian match qml.Hermitian in behaviour when it comes to wires.

pennylane/devices/default_qubit.py Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
tests/ops/test_qubit_ops.py Outdated Show resolved Hide resolved
@soranjh soranjh requested a review from josh146 June 25, 2021 13:44
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Nice, thanks @soranjh! I think it's very ready for merging, I just noticed a few very small things to change.

.github/CHANGELOG.md Outdated Show resolved Hide resolved
pennylane/devices/default_qubit.py Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/ops/qubit.py Outdated Show resolved Hide resolved
pennylane/qnode.py Show resolved Hide resolved
tests/ops/test_sparse.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ixfoduap ixfoduap left a comment

Choose a reason for hiding this comment

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

Good to go from my side, but please address the remaining suggestions from @josh146

@soranjh soranjh requested a review from josh146 June 25, 2021 22:08
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Thanks @soranjh! I'm happy to now approve this PR, however, I recommend updating the gradient test to test a case where the gradient is not zero. There have been cases in the past where our gradient tests have been using test circuits that were too trivial, leading to bug not being uncovered.

tests/ops/test_sparse.py Outdated Show resolved Hide resolved
@soranjh
Copy link
Contributor Author

soranjh commented Jun 28, 2021

Thanks @soranjh! I'm happy to now approve this PR, however, I recommend updating the gradient test to test a case where the gradient is not zero. There have been cases in the past where our gradient tests have been using test circuits that were too trivial, leading to bug not being uncovered.

Thanks @josh146. The gradient test is updated for a hydrogen Hamiltonian. Have you observed the codefactor issue "Complex Code" before? I computed the cyclomatic complexity with radon and everything seems normal in pennylane/ops/qubit.py, specially in my additions. Not sure which part of the code raises that issu.

@josh146
Copy link
Member

josh146 commented Jun 28, 2021

@soranjh I have overridden the complex code warning :)

@soranjh
Copy link
Contributor Author

soranjh commented Jun 28, 2021

@soranjh I have overridden the complex code warning :)

Thanks Josh!

@soranjh soranjh merged commit 5e1b832 into master Jun 29, 2021
@soranjh soranjh deleted the sparse_observable branch June 29, 2021 14:16
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.

None yet

3 participants