-
Notifications
You must be signed in to change notification settings - Fork 61
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
Save gates to JSON #1015
Save gates to JSON #1015
Conversation
Codecov ReportAll modified lines are covered by tests ✅
📢 Thoughts on this report? Let us know!. |
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.
The present one is just an addition, and it is not going to destroy anything, so for me is good to go.
I still believe we should improve the serialization, but most likely it will involve reworking a bit the Gate
class itself.
If you have anything that could be simple to add I can try to implement it as well. |
That's the main problem: it might not be too simple. |
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 for implementing this. It is a bit minimal and I am not sure if it works for all gates (maybe it will fail for Unitary
) but definitely better than nothing.
from typing import List, Sequence, Tuple | ||
|
||
import sympy | ||
|
||
from qibo.backends import GlobalBackend | ||
from qibo.config import raise_error | ||
|
||
REQUIRED_FIELDS = ["name", "init_kwargs", "_target_qubits", "_control_qubits"] | ||
REQUIRED_FIELDS_INIT_KWARGS = ["theta", "phi", "lam"] |
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.
You could include the trainable
here, right? Might be relevant for some applications, but I am not sure if those would use the serialization anyway.
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.
Yeah, I don't know either, for what I needed it wasn't necessary. This can extended as the application that requires it appears. I would try to save the least for now.
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.
Agreed, let's stick to something simple.
I want to rework the gates anyhow, and (if possible) write a simpler and more robust serialization. Maybe we will need to improve this in any case, but maybe not.
It does save the unitaries. In fact, most of the gates the RB saves are unitaries. |
Naive patch to save gates in RB implemented changes in qiboteam/qibocal#412.
Checklist: