-
Notifications
You must be signed in to change notification settings - Fork 191
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
Bosonic backend nongaussian #541
Conversation
Co-Authored-By: ilan-tz <57886357+ilan-tz@users.noreply.github.com> Co-Authored-By: Nicolas Quesada <991946+nquesada@users.noreply.github.com>
- Adds preparation methods for GKP, cat and Fock states - Adds the run_prog and init_circuit methods which are the engines for running sf Programs in the bosonic backend - Adds the frontend ops for calling these methods Co-Authored-By: ilan-tz <57886357+ilan-tz@users.noreply.github.com> Co-Authored-By: Nicolas Quesada <991946+nquesada@users.noreply.github.com> Co-Authored-By: GDauphinais <48553380+GDauphinais@users.noreply.github.com>
Tests for the bosonic backend
Codecov Report
@@ Coverage Diff @@
## master #541 +/- ##
==========================================
+ Coverage 98.15% 98.19% +0.04%
==========================================
Files 75 76 +1
Lines 8115 8327 +212
==========================================
+ Hits 7965 8177 +212
Misses 150 150
Continue to review full report at Codecov.
|
- Ensures that new modes can be added in program contexts, and that state preparation for the new mode can still be run - Adds an error if more than one non-Gaussian state prep is attempted in the same mode - Adds unit tests for the changes
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.
Mainly checked code quality, and had a few comments. Most fairly minor, or me just noticing small things that could be slightly improved. 🙂
GKP, | ||
Ket, | ||
MbSgate, | ||
_New_modes, |
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.
This class should probably be updated to _NewModes
to follow class naming convention.
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.
I'm just using what it was already called in the ops.py file.
_New_modes, | ||
) | ||
|
||
# Initialize the circuit. This applies all non-Gaussian state-prep |
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.
(very minor) I'm noticing that the comments sometimes are capitalized, sometimes not, and sometime use punctuation, sometimes not. It might be nice to use the same rules as we do with the docstrings: capitalize + punctuation when more than one sentence, otherwise all lowercase. Might just be me being a bit too picky here though. 😆
Co-authored-by: Theodor <theodor@xanadu.ai>
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 @elib20, left some comments. Looking awesome! 💯 😍
Nothing major, mostly suggestions related to code quality. Some cases where there still seems to be ambiguity is where there are multiple for
/if
nested blocks. In such cases may be worth trying to simplify them if easily feasible.
from strawberryfields.ops import ( | ||
Bosonic, | ||
Catstate, | ||
DensityMatrix, | ||
Fock, | ||
GKP, | ||
Ket, | ||
MbSgate, | ||
_New_modes, | ||
) |
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.
A personal note here (which is also relevant for other parts of the code base): the absolute import import strawberryfields.ops as ops
and prefixing each gate with ops.
should help with organizing the import to the top level without causing cyclic import issues. No need to change though, just a side note.
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.
This unfortunately didn't work :(
pars = cmd.op.p | ||
for reg in labels: | ||
# All the possible preparations should go in this loop | ||
if isinstance(cmd.op, Bosonic): |
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.
Would consider moving the branching if statement structure into a separate method if feasible (maybe not due to the new modes branch).
That would also help with:
- removing the too many branches linter check,
- shortening this method,
- including the comments as the docstring of the new method.
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.
Yes I think the _New_modes messes up that approach.
if val is not None: | ||
for i, r in enumerate(cmd.reg): | ||
if r.ind not in self.ancillae_samples_dict.keys(): | ||
self.ancillae_samples_dict[r.ind] = [val[:, i]] |
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.
Suggest checking if using a defaultdict
could help to decrease the nested nature of this part of the code (that would allow having each member to be initialized as an empty list by default, though suggest checking the quirks of it).
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.
Let's keep this on the list of things to investigate in the next round of improvements.
@@ -54,6 +69,205 @@ def __init__(self): | |||
self._supported["mixed_states"] = True | |||
self._init_modes = None | |||
self.circuit = None | |||
self.ancillae_samples_dict = {} | |||
|
|||
def run_prog(self, prog, **kwargs): |
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.
How come this method is defined? Mostly curious about how it is going to be used with the engine as other backends do not seem to define such a method.
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.
@antalszava : the idea here is that we want to have a custom run_prog
that checks the type of initialization that happens for the different modes. If you know in advance how the different modes are initialized then you can directly allocate the required arrays. Otherwise, everytime a new mode is parsed one would have to generate a new array with the extra dimensions required for the particular preparation. For example if you have a circuit with two cats, this method will recognize that it needs 4x4 = 16 coefficients, means and covariances. If you instead used the usual run_prog
you would first allocate 4 coefficients, means and covariances and then later when you read the preparation of the second cat you would have to generate the new arrays having a first index running from 0 to 16-1.
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.
It was decided early on to build a custom engine for the bosonic backend, which is essentially what this method is, mainly since we had some complications with the initializations for each mode. You will see the changes made to the engine file in the next PR. I'm tagging @ilan-tz since he can give a more thorough answer.
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.
Yep. The original backends (Fock, Gaussian, and TF) all are run 'step-by-step'. The LocalEngine
will initialize the backend with the number of modes and some other program independent properties, and will then separately loop over the program operations and call the corresponding backend methods.
This does not work well for the bosonic backend; information about the program is needed at initialization, but the current LocalEngine
does not provide! In fact, the LocalEngine
tries to do too much by itself.
A better approach is how the RemoteEngine
works - it passes the program directly to the backend, waits for the backend to interpret and execute it, and receives the results. This is the approach we have decided to go with for the Bosonic backend.
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.
Sidenote: we did look into attempting to modify the local engine to provide this information to the backend, but you end up going down a rabbit hole of needing to patch the local engine, patch the existing backends, and still have the whole thing look like a bit of a hotfix.
Co-authored-by: antalszava <antalszava@gmail.com> Co-authored-by: Josh Izaac <josh146@gmail.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.
@elib20 Looks good to me, amazing! 🥇 😍 So good to see this addition. 🙂
Co-authored-by: antalszava <antalszava@gmail.com>
Context: The bosonic backend simulates states as linear combinations of Gaussians in phase space
Description of the Change:
Benefits:
Possible Drawbacks:
Related GitHub Issues: