-
Notifications
You must be signed in to change notification settings - Fork 15
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
Porting emulator to 0.2 #1158
Porting emulator to 0.2 #1158
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1158 +/- ##
==========================================
- Coverage 52.65% 49.51% -3.15%
==========================================
Files 71 76 +5
Lines 3179 3381 +202
==========================================
Hits 1674 1674
- Misses 1505 1707 +202
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 code is already very clean, but here is a first batch of suggestions extracted while reading it.
They are all minor comments, other than the role of the embedded platform.
src/qibolab/_core/platform/load.py
Outdated
elif name == "emulator": | ||
from qibolab._core.emulator import create_emulator | ||
|
||
return create_emulator() |
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 know that dummy
is already an exception (and, possibly, we should also get rid of that one), but the emulator is even different from dummy
, since you may want to create multiple platforms, corresponding to different parameters (while dummy
was not supposed to be really used outside that single platform - but even that was naive...).
Practically, I'd propose to:
- drop
emulator/{platform.py,parameters.json}
(at least from insidesrc/
, possibly keeping them intests/
, if relevant) - scope the
emulator/
folder withininstruments/
, and treat it as a driver on its own - remove these lines from here, and rely on the default platform loading mechanism
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 have addressed all points in 513619f.
I wasn't sure where to put an example platform with the emulator, since probably qibolab_platforms_qrc
is not the ideal place given that we have only real platforms there. As a temporary solution in qiboteam/qibocal@47f0320 I have added a platforms
folder in Qibocal that loads all platforms inside at import time and I have defined there a platform emulator1q
. Other proposal are welcome.
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.
We used to have a /tests/dummy_qrc
for testing. We dropped it, since it was mostly outdated with the 0.2 transition (and, since long, we wanted to drop references to QRC from this repo - since it's just a special case).
I have in mind two main options:
- for tests or documentation purposes, you can certainly place a platform in here, either within
/tests/
or/doc/
themselves - for actual platforms, to be properly calibrated, we could actually have a
qibolab_platforms_emulators
repository
However, even the second option is mainly meant to make some emulator examples, and for common use cases.
Instead, for practical applications, the platform should be placed in the specific project where it is needed. E.g. as an aid (~digital twin) for QRC platforms calibration, it certainly makes sense to store them aside the other platforms they're emulating, i.e. in qibolab_platforms_qrc
. While for standalone projects with their own repos, those repos are actual the best location on their own.
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.
We used to have a
/tests/dummy_qrc
for testing. We dropped it, since it was mostly outdated with the 0.2 transition (and, since long, we wanted to drop references to QRC from this repo - since it's just a special case).
I remember that we used to have this folder in 0.1
, which was originally where the emulator platforms were stored https://github.com/qiboteam/qibolab/tree/0.1/tests/emulators
This is fine to store example or for testing purposes, however if we want to rely on the default platform loading mechanisms we will then need to copy and paste the platforms either in qibolab_platforms_qrc
or to another folder which will then need to add to QIBO_PLATFORMS
which could be annoying.
- for actual platforms, to be properly calibrated, we could actually have a
qibolab_platforms_emulators
repository
Personally I would avoid to add yet another repository. As you said it might be better to add the platforms directly in the project folder. Perhaps it might be useful to have an helper function in Qibolab to ease the addition of new platform folders, which is what I was trying to do directly in Qibocal.
6c70a89
to
513619f
Compare
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 went through everything but the EmulatorController
. However, as I wrote in a comment, I'm going to propose a separate array-oriented refactor for that. But it's just simpler to propose it as a commit.
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 the review @alecandido.
I agree with all the proposed changes, I'm only not sure about removing the assertion.
7eda97d
to
a849ef0
Compare
This PR attemps to port the emulator in 0.2.
Currently the only model supported is just a qubit without a resonator.