Support for QM OPX1000 in 0.2 #1068
Conversation
|
Following qiboteam/qibolab_platforms_qrc#188, this should now be ready. I also confirmed that the qw11q that is controlled with the old OPX+ cluster still works: http://login.qrccluster.com:9000/jnGzYDzWTuyMrezsF-khoA==/ |
There was a problem hiding this comment.
First round, still need to go through the details of the larger changes (config.py, devices.py, elements.py, controller.py).
In general, it is a bit larger change than what I expected at first, but still sufficiently localized.
We should be able to merge it soon.
No rush in terms of merging. It is also larger and required more effort than what I also expected. But this is mainly because it ended up not only for adding OPX1000 support, but also supporting its new features and doing workarounds around various bugs with (I believe) the qm-qua pre-release (the TODO list in the first comment). Two other things to note: I have changed a few type annotations from But a more important point to mention is about how OPX1000 controllers are exposed in terms of interface. This is more clearly visible in the platform, for example: https://github.com/qiboteam/qibolab_platforms_qrc/blob/7e2b2fb62300c9ba6e94f98c342597294cd4edab/qw5q_platinum/platform.py#L54 DcChannel(device="con1/4", path="1")and DcChannel(device="con1", path="4/1")The second is probably a bit better on the user end, since |
Yes, I agree with your analysis and decision. Essentially, it's about deciding whether a FEM is a device or a device's component. And that's ambiguous. It may be a good time to ask ourselves what is actually defining a device, and why each port is not a device on its own. Most likely, there was no problem until now, just because we were dealing with simpler devices (which at most had ports, or not even those). |
There was a problem hiding this comment.
Just minor suggestions, but in general it looks alright.
I'm still wondering whether we could simplify it a bit, since it may be quite nested at times (you could follow the story of max_voltage as a marker), but this would be a fully internal refactor - definitely not needed for this PR, and not even for a release.
| def _to_port(channel: Channel) -> dict[str, tuple[str, int]]: | ||
| """Convert a channel to the port dictionary required for the QUA config.""" | ||
| return {"port": (channel.device, channel.port)} | ||
| def _to_port(channel: Channel) -> dict[Port, InOutType]: |
There was a problem hiding this comment.
Concerning the return type hint, in this case it works fine because you only have one element. But it would not scale for a dictionary with multiple (inhomogeneous) entries.
In general, the tool to define these kinds of type hints (without promoting them to an actual dataclass), is to use a TypedDict.
You can define it inline (functional style) or with a dataclass-like syntax. Still it will be a hint for a plain dict, it does not require different runtime classes.
There was a problem hiding this comment.
Thanks for the suggestion, good to know. I updated several type hints in this file to use this in 4d00aeb, let me know if you agree.
| # default_factory=lambda: PortDict(DEFAULT_INPUTS) | ||
| # ) |
There was a problem hiding this comment.
Thanks for catching this. I decided to go with the original one (that is: not change in this PR), as I think the default value there was needed for something related to the mixer calibration. I just had to also add default offsets (see e4947d9) because qm-qua>1.2 fails without them.
|
@alecandido is there something wrong with pre-commit? It didn't complain to me locally. Other than that, if you are okay with the last changes we can probably merge. I did a quick test and it still works on hardware. |
Yes, it's broken because of a recent update (v3 -> v4) that propagated to the CI, but not all our hooks are supporting the update (it makes sense). However, there is only one hook failing, that is already patched in For the time being, please ignore it. |
Ports #1045 to qibolab 0.2.
TODO:
triggeredandalways_on)