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 function for convert circuit parameters to numpy #3899

Merged
merged 20 commits into from Apr 12, 2023

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Mar 14, 2023

Currently, we use the qml.tape.Unwrap context manager to convert parameters to numpy before sending them to a device.

This context manager modifies the input in-place and resets it at the end. It uses QuantumScript.set_parameters, which iterates over all operations, modifies them in place, and then checks batching.

Notably, it calls op._check_batching for every single parameter that gets set, not every operator that has parameter set.

This causes massive performance problems for large observables. We are also setting the parameters for operators that might not even need it.

This PR proposes a transform convert_to_numpy_parameters that creates a new quantum script with numpy or built-ins parameters. Currently, it is in the devices.preprocessing module, as it was designed thinking about new devices, and making it easy for new devices to convert to vanilla numpy parameters.

Given the huge performance problems with Unwrap, we should consider using this everywhere instead.

Screenshot 2023-03-14 at 10 48 22 AM

When using a Sum object instead of a Hamiltonian, Unwrap takes 1min 36seconds, a great deal more time. convert_to_numpy_parameters takes 8.42 ms, twice the amount of time as for Hamiltonian, but still substantially faster than the Unwrap case.

If we are going to immediately replace Unwrap with this instead, we should better consider the location for this conversion utility.

@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #3899 (54532f5) into master (e5f0476) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3899   +/-   ##
=======================================
  Coverage   99.56%   99.56%           
=======================================
  Files         348      349    +1     
  Lines       30382    30410   +28     
=======================================
+ Hits        30250    30278   +28     
  Misses        132      132           
Impacted Files Coverage Δ
pennylane/transforms/__init__.py 100.00% <100.00%> (ø)
...ennylane/transforms/convert_to_numpy_parameters.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@albi3ro albi3ro marked this pull request as ready for review April 3, 2023 15:42
@github-actions
Copy link
Contributor

github-actions bot commented Apr 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.

@albi3ro albi3ro requested review from timmysilv and a team April 10, 2023 13:58
@timmysilv
Copy link
Contributor

it's as if you magically looked at my local branch and nabbed the changes I made! looks excellent. I want to confirm a thing or two before approving, but I will likely approve today.

Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Thanks Christina! I think that this looks good. Is there a timeline for when this would replace tape.unwrap?

pennylane/transforms/convert_to_numpy_parameters.py Outdated Show resolved Hide resolved
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
@albi3ro
Copy link
Contributor Author

albi3ro commented Apr 11, 2023

[sc-36752]

@albi3ro
Copy link
Contributor Author

albi3ro commented Apr 11, 2023

Thanks Christina! I think that this looks good. Is there a timeline for when this would replace tape.unwrap?

@mudit2812 We are working on a PR to replace the use of unwrap in the interfaces module.

albi3ro and others added 2 commits April 11, 2023 16:18
Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
@albi3ro albi3ro enabled auto-merge (squash) April 12, 2023 18:11
@albi3ro albi3ro merged commit 7584b79 into master Apr 12, 2023
43 checks passed
@albi3ro albi3ro deleted the convert-to-numpy-parameters branch April 12, 2023 18:26
mudit2812 added a commit that referenced this pull request Apr 13, 2023
* convert to numpy parameters transform

* tests

* update math module

* move to transforms folder

* measurement test too

* Update pennylane/transforms/convert_to_numpy_parameters.py

Co-authored-by: Matthew Silverman <matthews@xanadu.ai>

* improve tests, copy over trainable_params

* Apply suggestions from code review

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>

* see also derective and tensor data

* fix docstring

* Update pennylane/transforms/convert_to_numpy_parameters.py

Co-authored-by: Matthew Silverman <matthews@xanadu.ai>

* tensor bugfix

---------

Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
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