-
Notifications
You must be signed in to change notification settings - Fork 17
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
Added device_setup to Zurich #664
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #664 +/- ##
==========================================
- Coverage 66.07% 66.06% -0.01%
==========================================
Files 57 57
Lines 7826 7819 -7
==========================================
- Hits 5171 5166 -5
+ Misses 2655 2653 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/qibolab/instruments/zhinst.py
Outdated
@@ -288,7 +288,7 @@ class Zurich(Controller): | |||
|
|||
PortType = ZhPort | |||
|
|||
def __init__(self, name, descriptor, use_emulation=False, time_of_flight=0.0, smearing=0.0): | |||
def __init__(self, name, descriptor=None, device_setup=None, use_emulation=False, time_of_flight=0.0, smearing=0.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.
suggestion: Instead of adding optional arguments, alternate constructors might make this interface cleaner. I suggest adding two class methods:
@classmethod
def from_descriptor(cls, descriptor: ?, name: str, ...):
...
@classmethod
def from_device_setup(cls, device_setup: DeviceSetup, name: str, ...):
...
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, my intention was only to temporarily keep backwards compatibility.
We should probably just drop descriptor
from the constructor (removing default None
from device_setup
), but I'd like to discuss it with @stavros11 and @Jacfomg.
@GabrielePalazzo can you have a look at the last failing test for the serialization ? |
Checklist: