Custom sampling rate for emulator #1325
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1325 +/- ##
==========================================
+ Coverage 40.15% 40.19% +0.03%
==========================================
Files 113 113
Lines 5531 5536 +5
==========================================
+ Hits 2221 2225 +4
- Misses 3310 3311 +1
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:
|
alecandido
left a comment
alecandido
left a comment
There was a problem hiding this comment.
Some minor comment, but this is already good. Thanks!
| class EmulatorController(Controller): | ||
| """Emulator controller.""" | ||
|
|
||
| sampling_rate_: float = Field(1, exclude=True) |
There was a problem hiding this comment.
Did you exclude for compatibility with the previous platforms? Is it required despite the default?
In case, I would just suggest not to worry too much about breaking. It is definitely better not to do it, but if that's happening rarely and in more and more isolated corners[*] it is more or less acceptable. We're not yet that stable, so it is fine to allow some wiggle room.
[*]: this not really a corner, but it would only break emulator-based platforms, at most (not even sure), and while the emulator is certainly useful, we never dedicated enough effort to claim that the overall structure is quite stable - at least, we could break something, if it's only happening seldom
There was a problem hiding this comment.
I did this in order to have the property and property.setter working below. I don't know if there is another way to make it working.
There was a problem hiding this comment.
Ah, I see.
Rodolfo was doing the same for the Qibosoq driver, but he did not need to exclude the field
qibolab/src/qibolab/_core/instruments/qibosoq/board.py
Lines 35 to 39 in 0da70b6
There was a problem hiding this comment.
I see, let me try to remove it to see if everything works as expected.
| @property | ||
| def sampling_rate(self) -> float: | ||
| return self.sampling_rate_ | ||
|
|
||
| @sampling_rate.setter | ||
| def sampling_rate(self, value: float): | ||
| self.sampling_rate_ = value |
There was a problem hiding this comment.
Yes, this is annoying - I need to investigate and solve this problem...
However, I was considering to completely drop the sampling_rate as an overall property. In principle, even within a controller, it may differ by physical port, or input vs output. #1292
What do you think about it?
There was a problem hiding this comment.
I don't have a strong preference, as long as we can set it in a more user-friendly way.
There was a problem hiding this comment.
The idea is that it will end up in the parameters.json, in particular in the configs. So, it will be as much inconvenient as anything else in there :P
(at some point we could make both documentation for the parameters.json structure, which could be generated automatically through Pydantic and JSON schema-based docs frameworks, and a more user-friendly interface to generate updates - at that point, even the sampling rate will benefit of those)
This PR adds the possibility to set the sampling rate for the emulator, which is particularly useful for running experiments that requires higher sampling rate.