-
Notifications
You must be signed in to change notification settings - Fork 60
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
Backend abstraction #303
Backend abstraction #303
Conversation
@stavros11 thanks for this. It implements exactly what I had in mind and I hope it simplifies the code. |
That's a good suggestion. My idea is to first try and refactor the Qibo code (circuits, models, Hamiltonians, etc.) to use Right now I am not sure what is the best approach in terms of module structure. I think this should be built in terms of dependencies. One possibility is to have the base independent of all external libraries and have a different folder for the models that actually do the calculation (similar to what we have now, although currently base depends on numpy and I am not sure if it is a good idea to remove that dependency). |
src/qibo/core/cgates.py
Outdated
theta = self.parameters | ||
cos, sin = np.cos(theta / 2.0), np.sin(theta / 2.0) | ||
return np.array([[cos, -sin], [sin, cos]], dtype=DTYPES.get('NPTYPECPX')) | ||
cos, sin = qnp.cos(theta / 2.0), qnp.sin(theta / 2.0) |
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.
Ok, that's a good point. So ideally the code is trying to do:
- use math for constants and expected python numerical types
- use numpy (
from qibo import numpy
) for objects which must be or are expected to be numpy objects - use K in places where objects rely on the typo of the state.
The idea is good, however in places like here, it supposes the parameters will be as numpy arrays or compatible formats (tf which casts to numpy) or python lists. I am fine with these choices, but would be great to have some place where we summarize how to use K
, qibo.numpy
and math
.
src/qibo/hamiltonians.py
Outdated
hz = np.kron(matrices.Z, matrices.Z) | ||
hx = qnp.kron(matrices.X, matrices.X) | ||
hy = qnp.kron(matrices.Y, matrices.Y) | ||
hz = qnp.kron(matrices.Z, matrices.Z) |
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.
For example here, if we implement jax we may prefer to use K.kron in order to use the GPU allocation.
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 is just a comment, from a practical perspective we don't need this level of flexibility for these objects.
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.
@stavros11 thank you very much for this masterpiece!
The implementation looks very good to me, and matches the layout needed by our projects.
You can find some minor comments below, but in general looks pretty good, my only concern is about the usage documentation of math
, K
, qnp
.
This PR with #293 and eventually a new state interface #300 should be sufficient for the time being.
Thank you for the detailed review. I fixed some of the above comments. I also removed I will continue working on the remaining comments and will try to also improve documentation. |
I believe I fixed most of the above comments except the following two: First, regarding the initial state custom operator in the defaulteinsum/matmuleinsum, as I wrote above we could probably do the state initialization using Tensorflow primmitives for these backends so that we keep them completely custom operator free. Second, regarding the
Regarding The difference between |
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.
@stavros11 thanks for the updates.
First, regarding the initial state custom operator in the defaulteinsum/matmuleinsum, as I wrote above we could probably do the state initialization using Tensorflow primmitives for these backends so that we keep them completely custom operator free.
Lets keep the custom init for all tf backends.
The difference between K.matmul and K.np.matmul would be that the first would be either numpy or tf, while the second will be forced to numpy for all K. If we add a different backend (eg. Jax) that is fast for both small and big operations, we could define its K.np to be Jax and in this case both K.matmul and K.np.matmul would be Jax.
I tend to prefer this idea, so we reduce the number of objects which may refer to numpy but maybe not, say with jax, or tf.numpy.
Some other comments when testing the code:
- would be great to have a more helpful error message for
set_backend(backend)
ifbackend
is not supported. Ideally we could simply print the available backend names. - if
qibo.K.name == 'numpy'
and I try toset_device('/device:CPU:0')
(same for GPU), I get "Device /device:CPU:0" does not exist". This error message may be misleading, so we could simply raise a warning message, saying that numpy does not support CPU:0 e GPU:0, or keep the default not implemented error.
I replaced
I updated this error message and now it prints all the available backend names.
I disabled |
@stavros11 thank you, everything looks good. Let's wait for the test and then merge. |
This is a toy implementation of what is discussed in #300 regarding the numpy/tensorflow backend abstraction and switcher. @scarrazza I used the idea you proposed for the switcher with a few simplifications. I tested with the following script:
and the switcher seems to work as expected.
Please have a look and if you agree I can add the rest of methods we need in the backends and then think how to integrate it with the rest of Qibo. Ideally we would like almost the whole Qibo code (perhaps except gates) to be free of
tf
calls and useK
instead.