Unrolling bounds as instrument settings #832
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #832 +/- ##
==========================================
- Coverage 66.44% 66.35% -0.09%
==========================================
Files 50 50
Lines 6079 6063 -16
==========================================
- Hits 4039 4023 -16
Misses 2040 2040
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| self.bounds = Bounds( | ||
| waveforms=int(4e4), | ||
| readout=250, | ||
| instructions=int(1e6), |
There was a problem hiding this comment.
Where do these numbers come from. I believe ZI limitations are much larger than these numbers.
Additionally, different ZI instruments have different limitations. Our Zurich class does not represent a single instrument, but a collection of ZI instruments. So the concept of bounds is not straightforwardly applicable to this class. The best we can do for now is for each property take the minimum across all possible ZI instruments.
There was a problem hiding this comment.
In principle, we should be able to serialize the configuration for every instrument in the platform. They should just live in nested sections.
There was a problem hiding this comment.
qibolab/src/qibolab/serialize.py
Line 174 in adfc27d
There was a problem hiding this comment.
I ran some quick experiments until they crashed for the waveforms and readouts, I don't claim them to be the best but I needed something for the code. They should get updated with the feedback from people running stuff I hope.
There was a problem hiding this comment.
Thanks @stavros11 for having propagated this to all drivers.
There was a problem hiding this comment.
I would set a random one also here, with a # TODO: marker. At least, whoever will try to use sweepers on the rfsoc will notice that there is something to be done here, and doesn't have to look too much around.
I would just set something like (1000, 1000, 1000), to fully prevent batching. It will make it fail for memory, but it is better than completely prevent unrolling, isn't it?
There was a problem hiding this comment.
I am actually not sure how qibosoq (and QICK) would behave regarding unrolling. I remember some issues when sending long sequences (not necessarily unrolled ones, just a long sequence with a single measurement) in which case there was no error but the results were wrong. If that is the case, it may be actually better to prevent unrolling.
I believe now Bounds(0, 0, 0) is inherited from the abstract instrument, so every sequence should be batched individually.
| settings = instrument.dump() | ||
| if len(settings) > 0: | ||
| data[name] = settings | ||
| except AttributeError: |
There was a problem hiding this comment.
Is there anyone still missing .dump(), but with .settings?
There was a problem hiding this comment.
Yes, I believe the qblox modules, not the controller itself but QrmRf, QcmRf, etc. And in the platforms we need to pass settings to each module seperately.
I believe this is not necessary. If no bounds are given in parameters.json it will use the defaults hardcoded for the used instrument. These are the values from your PR which I believe are good estimates for what we know so far. If someone finds better values then they can put them in the platform. The only confusion may be that platforms dumped by qibocal will now contain the default bounds (even when not specified in the original platform), but that shouldn't be an issue. @alecandido I fixed the conflict and responded to your comments. If you have no objection, feel free to merge. Regarding |
|
I manually changed base branch to the PR. @scarrazza is it possible to enable to automated rebase in the settings? |
For this we should decide a consistent scheme, since those platforms are becoming lock files, but they should not be recommitted as is to the platforms' repo (otherwise it would not make use of the defaults any longer). |
Implements what is described in #817 (comment).