Skip to content

Batched csr in ParametrizedEvolution with jax version bump#4126

Merged
Qottmann merged 27 commits intomasterfrom
batchedCSR
May 18, 2023
Merged

Batched csr in ParametrizedEvolution with jax version bump#4126
Qottmann merged 27 commits intomasterfrom
batchedCSR

Conversation

@Qottmann
Copy link
Copy Markdown
Contributor

@Qottmann Qottmann commented May 15, 2023

Following #4079 which hot-fixes #4078, we can now fix it for good with batched CSR. This update is necessary anyway since "vanilla" CSR format will be deprecated soon (see jax-ml/jax#15844 (comment))

@Qottmann Qottmann requested a review from rmoyard May 18, 2023 09:23
@Qottmann Qottmann changed the title Batched csr Batched csr in ParametrizedEvolution with jax version bump May 18, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented May 18, 2023

Codecov Report

Merging #4126 (3086b56) into master (d95ae12) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4126   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files         340      340           
  Lines       30523    30523           
=======================================
  Hits        30456    30456           
  Misses         67       67           
Impacted Files Coverage Δ
pennylane/pulse/parametrized_hamiltonian_pytree.py 100.00% <100.00%> (ø)

Copy link
Copy Markdown
Contributor

@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.

Nice change, I like how minimal it is while unlocking quite a bit of performance in the long run :)
Just wondering whether we

  1. may break anything with the new doc/requirements settings, and
  2. should add a test that makes use of a particular BCSR feature that CSR would fail.
    For the second, I think it is not really necessary, but it might be nice.

Copy link
Copy Markdown
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks @Qottmann it looks good to me 👍

Copy link
Copy Markdown
Contributor

@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.

Looks good to me, thanks @Qottmann ! 🎉

@Qottmann
Copy link
Copy Markdown
Contributor Author

should add a test that makes use of a particular BCSR feature that CSR would fail.
For the second, I think it is not really necessary, but it might be nice.

Yes good point! Testing now differentiating the unitary matrix with jit, no jit, dense, and sparse :)

@Qottmann Qottmann enabled auto-merge (squash) May 18, 2023 15:49
@Qottmann Qottmann merged commit a26db18 into master May 18, 2023
@Qottmann Qottmann deleted the batchedCSR branch May 18, 2023 16: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.

Cannot differentiate the unitary ParametrizedEvolution when sparse is active

3 participants