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.

Sign up for GitHub

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

Jump to bottom

Qchem-ops broadcasting support #2726

Merged
merged 158 commits into from
Aug 3, 2022
Merged

Qchem-ops broadcasting support #2726

merged 158 commits into from
Aug 3, 2022

Conversation

dwierichs
Copy link
Contributor

@dwierichs dwierichs commented Jun 16, 2022 •
edited
Loading

Here we adapt the numerical representation compute_matrix of the quantum chemistry operations to support parameter broadcasting, in analogy to #2609.

dwierichs and others added 30 commits May 15, 2022 14:00
@dwierichs
introduce Operator.ndim_params, Operator.batch_size, QuantumTape.batc…
14051d1 
…h_size
@dwierichs
linting
6a3e484
@dwierichs
Merge branch 'master' into parameter-broadcasting-0
b50a84f
@dwierichs
changelog
6ef28f0
@dwierichs
enable tf.function input_signature usage
4a7fea2
@dwierichs
black
d95c12b
@dwierichs
test for unsilenced error
ecf7acc
@dwierichs
Merge branch 'master' into parameter-broadcasting-0
24b0c1b
@dwierichs @josh146
Apply suggestions from code review
82a18fc 
Co-authored-by: Josh Izaac <josh146@gmail.com>
@dwierichs
introduce device flag and batch_transform for unbroadcasting; use tra…
3ee5643 
…nsform in device.batch_transform
@dwierichs
black, [skip ci]
9e44a74
@dwierichs
code review
5bde6c7
@dwierichs
string formatting [skip ci]
9d8f726
@dwierichs
operation broadcasting interface tests
f550d00
@dwierichs
unbroadcast_expand
ad6022c
@dwierichs
tests for expand function
809b4fc
@dwierichs
tests
e38afc8
@dwierichs
black
46934d4
@dwierichs
compatibility with TensorFlow 2.6
b7f3570
@dwierichs
Merge branch 'master' into parameter-broadcasting-0
1dfedd2
@dwierichs
Merge branch 'parameter-broadcasting-0' into parameter-broadcasting-1
f164590
@dwierichs
builtins unstack
5389f34
@dwierichs
failing case coverage
733a852
@dwierichs
Merge branch 'master' into parameter-broadcasting-0
9f94f7c
@dwierichs
stop using I in operation.py [skip ci]
c3c24dd
@dwierichs
Merge branch 'parameter-broadcasting-0' into parameter-broadcasting-1
8960bbb
@dwierichs
commit old changes
d317048
@dwierichs @josh146
Apply suggestions from code review
89ef530 
Co-authored-by: Josh Izaac <josh146@gmail.com>
@dwierichs
intermed
dde9478
@dwierichs
review
68084cc
@dwierichs dwierichs changed the title Qchem-ops broadcasting support and speedup of parametric ops Qchem-ops broadcasting support Jun 21, 2022
@rmoyard rmoyard added this to the v0.25.0 milestone Jul 15, 2022
@AmintorDusko AmintorDusko self-requested a review July 18, 2022 14:02
@AmintorDusko AmintorDusko removed their request for review July 19, 2022 13:09
@rmoyard rmoyard removed this from the v0.25.0 milestone Jul 19, 2022
@josh146 josh146 requested a review from soranjh July 27, 2022 19:15
soranjh
soranjh reviewed Jul 28, 2022
View reviewed changes
Copy link
Contributor

@soranjh soranjh left a comment

Choose a reason for hiding this comment

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

Thanks @dwierichs, the changes look good but I am curious if such extensions/complications of the functions really beneficial? Do they improve performance? Also, can the new changes become more compact? I see some repetitions that could potentially be factored out to a new function (not sure about this though)?

pennylane/ops/qubit/qchem_ops.py Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Outdated Show resolved Hide resolved
dwierichs
dwierichs commented Aug 1, 2022
View reviewed changes
Copy link
Contributor Author

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

Thanks for the review @soranjh!
I hope I could answer the questions :)

pennylane/ops/qubit/qchem_ops.py Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Outdated Show resolved Hide resolved
pennylane/ops/qubit/qchem_ops.py Outdated Show resolved Hide resolved
@dwierichs
Copy link
Contributor Author

I now introduced a unifying function for both, the SingleExcitation operation and its variants, and the DoubleExcitation and its variants. This was a very nice call, @soranjh, thanks! :) Reusing the same function for all three respective variants was very easy to achieve and reduces the code complexity in the file a good bit.

@dwierichs dwierichs merged commit f3005be into master Aug 3, 2022
@dwierichs dwierichs deleted the qchem-broadcasting branch August 3, 2022 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soranjh soranjh soranjh approved these changes

@antalszava antalszava Awaiting requested review from antalszava

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dwierichs @soranjh @rmoyard