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

Enable dense keyword in ParametrizedEvolution #4079

Merged
merged 12 commits into from May 4, 2023
Merged

Conversation

Qottmann
Copy link
Contributor

@Qottmann Qottmann commented May 3, 2023

Hot fix for #4078 (see also google/jax#15844)

Enables a keyword dense to be passed to ParametrizedEvolution that enforces dense evolution, which currently automatically is enabled for more than 2 wires

Edit2: will have to wait for jax 0.4.9 support to enable BCSR, so this is a hotfix for the moment (see discussion)

@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2023

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.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.

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #4079 (02c4e1e) into master (755b241) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4079   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files         345      345           
  Lines       30737    30738    +1     
=======================================
+ Hits        30650    30651    +1     
  Misses         87       87           
Impacted Files Coverage Δ
pennylane/pulse/parametrized_evolution.py 95.94% <100.00%> (+0.05%) ⬆️

@PhilipVinc PhilipVinc self-requested a review May 3, 2023 12:32
@Qottmann
Copy link
Contributor Author

Qottmann commented May 3, 2023

Implemented the fix from google/jax#15844 and #4078

In case that solves it (and doesnt cause any unexpected behavior / failing tests), would we still want the possibility to enforce dense matrices?

@Qottmann Qottmann changed the title enable dense matrices in ParametrizedEvolution operator Enable batched csr matrix multiplications in ParametrizedEvolution May 3, 2023
pennylane/pulse/parametrized_evolution.py Outdated Show resolved Hide resolved
@PhilipVinc
Copy link
Contributor

Can we also have a test checking that specifying dense=True works?

@PhilipVinc
Copy link
Contributor

cause any unexpected behavior / failing tests), would we still want the possibility to enforce dense matrices?

I'd say that's useful. Can be deprecated in the future if we don't want this to be an option anymore.

@Qottmann
Copy link
Contributor Author

Qottmann commented May 3, 2023

This is blocked because we are stuck at 0.4.3 jax version. The main problem has been fixed on the jax side in google/jax#14833 but we would also need to drop python 3.8 and that may take a bit longer 🥲 CC @rmoyard

@mlxd
Copy link
Member

mlxd commented May 3, 2023

Just a fly-by comment: once this PR is ready to go can we enable the GPU CI tests by adding the "gpu" label to this PR?
To avoid overheads, I'd consider this only when the work is ready to merge.

@Qottmann
Copy link
Contributor Author

Qottmann commented May 3, 2023

Thanks, would have forgotten that! Are GPU tests running all jax tests with gpu enabled? 🤩 I always wanted to test the pulse stuff on GPU but never had the occasion

@rmoyard
Copy link
Contributor

rmoyard commented May 3, 2023

The GPU tests for the moment only have Torch.

@mlxd
Copy link
Member

mlxd commented May 3, 2023

Yep, as @rmoyard said only torch is supported now. We should ensure the runner installs the JAX CUDA version for this to be supported.

@dwierichs dwierichs self-requested a review May 4, 2023 12:45
@Qottmann Qottmann changed the title Enable batched csr matrix multiplications in ParametrizedEvolution Enable dense keyword in ParametrizedEvolution May 4, 2023
Copy link
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.

Thanks for the quick fix @Qottmann 💪
Three small comments :)

pennylane/pulse/parametrized_evolution.py Show resolved Hide resolved
pennylane/pulse/parametrized_evolution.py Outdated Show resolved Hide resolved
tests/pulse/test_parametrized_evolution.py Show resolved Hide resolved
Copy link
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.

Thanks @Qottmann !

@Qottmann
Copy link
Contributor Author

Qottmann commented May 4, 2023

Merging this and waiting with the change from csr to batched csr format when we have jax 0.4.9 supported in PL

@Qottmann Qottmann merged commit 85bbc0e into master May 4, 2023
43 checks passed
@Qottmann Qottmann deleted the dense-evolution branch May 4, 2023 15:08
brownj85 pushed a commit that referenced this pull request May 4, 2023
* enable dense matrices in ParametrizedEvolution operator

* doc arg description

* CSR -> BCSR

* revert BCSR -> CSR due to being stuck on jax version 0.4.3 for the moment

* add test for coverage

* black

* pylint

* code review suggestion

* test case
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

5 participants