-
Notifications
You must be signed in to change notification settings - Fork 585
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
Restructures preprocess
to include building block, extensible transforms
#4659
Conversation
Hello. You may have forgotten to update the changelog!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments and fixes for typos.
Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>
[sc-47192] |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4659 +/- ##
=======================================
Coverage 99.64% 99.64%
=======================================
Files 376 376
Lines 33810 33828 +18
=======================================
+ Hits 33689 33707 +18
Misses 121 121
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @albi3ro! I think this is great, but we need to polish the docstrings with an explanation of what each transform does, when a dev would use it, and some examples.
Also a more philosophical question - why do we need these to be transforms?
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
These will be objects in a We can either use
The option we're using right now just looks a bit cleaner. |
…ennylane into cleaning-preproces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @albi3ro, I have a few more suggestions.
tests/devices/default_qubit/test_default_qubit_preprocessing.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>
Co-authored-by: Amintor Dusko <87949283+AmintorDusko@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @albi3ro, thank you for that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing on it after everyone else and nothing to report. Looks good to me! Thanks @albi3ro
The current
devices.qubit.preprocess
module has several transforms optimized for default qubit. Other devices would need to rewrite similar code in order to extend the behavior.This PR restructures the module so that
devices.preprocess
now offers the following transforms:decompose
: this accepts a stopping condition and decomposes operations until the stopping condition is metvalidate_measurements
: this accepts a stopping_condition for observables. It validates all observables are accepted, only sample based measurements are present when sampling, and only state based measurements are present when analyticvalidate_device_wires
: This makes sure device wire constraints are met, and fills in all wires for measurements likeStateMP
andProbabilityMP
that act on all available wires.validate_multiprocessing_workers
: Checks that the CPU is not oversubscribed for a given number of workers.warn_about_trainable_observables
: raises a warning is any observable are trainableFor example, a plugin with a list of supported operations can use the
decompose
transform by doing:Overview of all the changes:
So this PR ended up changing a lot of lines of code, but most of them are moving tests around.
As the tests for default qubit started being quite extensive, I added a folder
tests/devices/default_qubit
and moved the default qubit tests there over two files.Any test for default qubit specific preprocessing went into
tests/devices/default_qubit/test_default_qubit_preprocessing.py
.More general preprocessing tests are in
tests/devices/test_preprocessing.py
.Since the transforms are no longer default qubit specific, they are moved out of the
qubit
folder.Since
DefaultQubit.preprocess
was getting rather long, I split out two private helper methods,_add_adjoint_transforms
and_setup_execution_config
. This change just keepsDefaultQubit.preprocess
a little more manage able.