Simplify and move Qblox modules settings #640
Conversation
PiergiorgioButtarini
requested review from
andrea-pasquale and
Edoardo-Pedicillo
There was a problem hiding this comment.
Thanks for implementing this. I will have a look in more detail in the following days, just noting that you can also update the qblox platform/runcard in tests/dummy_qrc. This is required for tests to pass with the new approach and can also serve as a template for the new structure of qblox runcards.
PiergiorgioButtarini
changed the title
There was a problem hiding this comment.
Thanks for opening the PR @PiergiorgioButtarini.
Please find below some comments (mainly removing commented code and a few questions).
I did not test the code yet with qibocal.
| """Initialises the instance. | ||
|
|
||
| All class attributes are defined and initialised. | ||
| """ | ||
|
|
||
| super().__init__(name, address) | ||
| self.settings: ClusterQRM_RF_Settings = settings | ||
| # self.settings: ClusterQRM_RF_Settings = settings |
| # self.connect() | ||
| # if not settings: | ||
| # log.warning(f'Called setup() without passing parameters for module {self.name}') | ||
| # return |
| # else: | ||
| # raise Exception("The instrument cannot be set up, there is no connection") |
| self.ports["o1"].lo_enabled = True | ||
| self.ports["o1"].lo_frequency = self._settings["o1"]["lo_frequency"] | ||
| self.ports["o1"].gain = self._settings["o1"]["gain"] | ||
| # self.ports["o1"].hardware_mod_en = port_settings.hardware_mod_en |
| # for sweeper in sweepers: | ||
| # if sweeper.parameter is Parameter.amplitude: | ||
| # # qblox cannot sweep amplitude in real time, but sweeping gain is quivalent | ||
| # for pulse in sweeper.pulses: | ||
| # pulse.amplitude = 1 | ||
|
|
||
| # elif sweeper.parameter is Parameter.gain: | ||
| # for pulse in sweeper.pulses: | ||
| # # qblox has an external and an internal gains | ||
| # # when sweeping the internal, set the external to 1 | ||
| # # TODO check if it needs to be restored after execution | ||
| # if pulse.type == PulseType.READOUT: | ||
| # qubits[pulse.qubit].readout.gain = 1 | ||
| # elif pulse.type == PulseType.DRIVE: | ||
| # qubits[pulse.qubit].drive.gain = 1 |
There was a problem hiding this comment.
I know that this was already commented but if it is not used I suggest to remove it.
There was a problem hiding this comment.
I would like to speak with Alvaro before removing this.
There was a problem hiding this comment.
I would also suggest to stick to one thing per PR so that it is easier to review and fix any introduced bugs. Since this PR is about the instrument settings, avoid touching the sweepers as much as possible, even if you see things that you don't like. If it's small fixes, fine, but if it's a big change it may introduce some bug which will be harder to solve the bigger the PR is (and the settings part is already big).
| Args: | ||
| name(str): A unique name given to the instrument. | ||
| address: IP_address:module_number (the IP address of the cluster and module number) | ||
| cluster: the cluster to which the instrument belongs. |
There was a problem hiding this comment.
Can we please add the correct type hint also in the docstring
There was a problem hiding this comment.
I would actually suggest avoiding type hints in the docstrings, just because they are now redundant with the actual hints, and it's more surface to be kept up to date (Sphinx is also fetching the parameter hints, so they will always be displayed).
In general, given the transition to explicit types, we do not necessarily need structured docstrings everywhere.
If possible, just keep few parameters in each function, and you can adopt a more conversational style (like it happens in Python docs, below some examples).
https://docs.python.org/3/library/shutil.html?highlight=shutils#shutil.copy2
https://docs.python.org/3/library/pathlib.html#pathlib.PurePath
https://docs.python.org/3/library/subprocess.html#subprocess.run
(even though they do not have the explicit types :P)
Examples are always welcome :)
There was a problem hiding this comment.
In your opinion something like this can work?
def __init__(self, name: str, address: str, cluster: Cluster = None):
"""
Initialize a Qblox QCM baseband module.
Parameters:
- name: An arbitrary name to identify the module.
- address: The network address of the instrument, specified as "cluster_IP:module_slot_idx".
- cluster: The Cluster object to which the QCM baseband module is connected.
Example:
To create a ClusterQCM_BB instance named 'qcm_bb' connected to slot 2 of a Cluster at address '192.168.1.100':
>>> cluster_instance = Cluster("cluster","192.168.1.100", settings)
>>> qcm_module = ClusterQCM_BB(name="qcm_bb", address="192.168.1.100:2", cluster=cluster_instance)
"""| # self.connect() | ||
| # if not settings: | ||
| # log.warning(f'Called setup() without passing parameters for module {self.name}') | ||
| # return | ||
|
|
| self.ports[port].gain = self._settings[port]["gain"] | ||
| self.ports[port].offset = self._settings[port]["offset"] | ||
| self.ports[port].qubit = self._settings[port]["qubit"] | ||
| # self.ports[port].hardware_mod_en = settings.hardware_mod_en |
| # if not settings: | ||
| # log.warning(f'Called setup() without passing parameters for module {self.name}') | ||
| # return |
| self.ports[port].lo_frequency = self._settings[port]["lo_frequency"] | ||
| self.ports[port].attenuation = self._settings[port]["attenuation"] | ||
| self.ports[port].gain = self._settings[port]["gain"] | ||
| # selected_port.hardware_mod_en = port_settings["hardware_mod_en"] |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #640 +/- ##
==========================================
+ Coverage 65.51% 65.94% +0.43%
==========================================
Files 57 57
Lines 7928 7817 -111
==========================================
- Hits 5194 5155 -39
+ Misses 2734 2662 -72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for working on this @PiergiorgioButtarini. I did not check all the changes in detail, but there are a few general things I would like to point out. I have mainly looked at cluster_qrm_rf.py for this, which I believe is the most complicated module, but I am assuming these observations can be somewhat generalized/transfered to the other modules as well.
The general hierarchy I have in mind is module -> ports -> settings dataclasses. The ports also provide an interface to change settings directly from Python (this is already implemented with the getters/setters in port.py). The workflow is the following:
- In the platform
createyou load the runcard and you callmodule.setup(viaload_instrument_settings). This parses the settings to the dataclasses under the ports. At this stage the settings are just cached as Python variables, not uploaded to the instrument as you are still not connected, - when
platform.connect()is called you connect to the instrument and then you should upload all the settings that are cached under the ports, - if the users changes some setting using the port interface while you are connected, the change should be immediately uploaded to the instrument (I believe this is implemented already),
platform.setup()should be useless at this stage, as it was already done during creation, and we will deprecate at some point.
That being said, starting from main (not this branch), I believe there are several attributes of ClusterQRM_RF that are related to the port/settings business and we can clean them up. These are:
self._input_port_keys: I believe it is nowhere used.self._output_port_keys: This is used in a few places but it can easily be replaced by something likeif isinstance(port, QbloxOutputPort)etc. Or easier, instead of having aself.portsdictionary with all the ports, you could split in twoself._input_portsandself._output_portsand make a property
@property
def ports(self):
return self._input_ports | self._output_portsself._device_parameters: This is just caching the module settings in Python, but we already have the caching mechanism with ports/settings, so we don't need it twice. If there are parameters that exist inself._device_parametersbut not in in the settings objects we should move them to settings. We can keep the same default values so that we don't have to set them in the runcard. It is just easier from code and documentation perspective to have all the parameters in a single place.self.channels: I believe it is nowhere used and in the current qibolab design instruments do not need to hold references to channels because they are accessible through theQubitobjects.self._channel_port_map: It is only used to createself.channelsand in theget_iffunction where it can be replaced by using theQubitobjects.self._port_channel_mapit is only used to createself._channel_port_mapand inprocess_pulse_sequencewhere it can be replaced by using theQubitobjects.- The methods
_set_device_parameterand_erase_device_parameterscan also be dropped if we removeself._device_parameters. If_set_device_parametersis useful for the port setters we can redefine it as a standalone utility function that can be reused among all modules.
These are quite high level suggestions, I have not really tested if we can safely drop all these without breaking something important. If you agree, I would suggest to also sit together for sometime and have a look if there are any issues.
| # for sweeper in sweepers: | ||
| # if sweeper.parameter is Parameter.amplitude: | ||
| # # qblox cannot sweep amplitude in real time, but sweeping gain is quivalent | ||
| # for pulse in sweeper.pulses: | ||
| # pulse.amplitude = 1 | ||
|
|
||
| # elif sweeper.parameter is Parameter.gain: | ||
| # for pulse in sweeper.pulses: | ||
| # # qblox has an external and an internal gains | ||
| # # when sweeping the internal, set the external to 1 | ||
| # # TODO check if it needs to be restored after execution | ||
| # if pulse.type == PulseType.READOUT: | ||
| # qubits[pulse.qubit].readout.gain = 1 | ||
| # elif pulse.type == PulseType.DRIVE: | ||
| # qubits[pulse.qubit].drive.gain = 1 |
There was a problem hiding this comment.
I would also suggest to stick to one thing per PR so that it is easier to review and fix any introduced bugs. Since this PR is about the instrument settings, avoid touching the sweepers as much as possible, even if you see things that you don't like. If it's small fixes, fine, but if it's a big change it may introduce some bug which will be harder to solve the bigger the PR is (and the settings part is already big).
| if self.is_connected: | ||
| try: | ||
| if "o1" in self.settings: | ||
| self.ports["o1"].attenuation = self.settings["o1"]["attenuation"] |
There was a problem hiding this comment.
As in the previous comment, here there is also some duplication with self.settings. In principle you should have the same attenuation value cached under self.ports["o1"]._settings.attenuation (or something like that).
| def instantiate_channels(channel_name: str, module, module_port_name: str): | ||
| module.channels.append(channel_name) | ||
| module._port_channel_map[module_port_name] = channel_name | ||
| module._channel_port_map[channel_name] = module_port_name | ||
| return Channel(name=channel_name, port=module.ports[module_port_name]) |
There was a problem hiding this comment.
The main reason I want to drop _port_channel_map and _channel_port_map is to get rid of this function. From the user perspective (the one that is writing the create method) it should be sufficient to define the channels and map them to ports, ie
channels["L3-15"] = Channel(name="L3-15", port=qcm_rf0.ports["o1"])This contains all the information we need from the lab configuration. The rest are manipulations of the channel map that we can do internally and only when needed.
RequirementThis branch works with the qblox runcard of PR qiboteam/qibolab_platforms_qrc#75. CommentsThis initial refactoring where the settings - tunable by some routine - of each qblox module are moved in the runcard yaml is working properly. In order to facilitate the review and debugging process I will implement the further refactoring pointed out by @stavros11 in other sub-branches. |
| @@ -119,8 +119,11 @@ def dump_qubits(qubits: QubitMap, pairs: QubitPairMap, couplers: CouplerMap = No | |||
|
|
|||
| def dump_instruments(instruments: InstrumentMap) -> dict: | |||
| """Dump instrument settings to a dictionary following the runcard format.""" | |||
| # Qblox modules settings are dictionaries and not dataclasses | |||
There was a problem hiding this comment.
Please, open an issue to trace the migration (unless you plan to migrate in this PR)
There was a problem hiding this comment.
About this I'm not sure I understood your comment. So for now I put this patch that works both for qblox and the other instruments, but you want that I open an issue in order to migrate in the future the settings of qblox from dictionaries to dataclasses?
There was a problem hiding this comment.
Indeed, that's exactly the request.
The idea is that we were migrating to dataclasses, and that is the interface we'd like to maintain. Fine to have a temporary patch, but (at least in the future) Qblox should implement the interface, instead of just giving up on the interface we established.
There was a problem hiding this comment.
Ok perfect thanks. I will open an issue about that.
Instead of making sub-branches, I would suggest attempting to merge this PR, and whatever is postponed will be added in further PRs (but, if possible, all pointing to |
Thank you @alecandido for the review, I will implement your suggestion asap. |
Generally I would agree with this, however for this particular case I suggested to hold (not merge) this PR even if it is working, because it requires changes to the qblox platforms (see qiboteam/qibolab_platforms_qrc#75). Implementing my suggestions would require further changes to the platforms (hopefully towards simplifying). And right now we have several (non-developer) users who using qibolab from main, so every time we merge something that requires changes to the platform creation, we need to properly communicate this. I would prefer to only communicate the final most simplified version, once we have it and we are confident with it. I guess the main flaw here is that users who are not developing are using main (instead of releases) but until the releases are sufficiently stable it is hard to convince people to use them in practice. The sub-branches idea is not strictly required, in principle we can push everything directly here until we are ready to merge, but it may help to keep the current status of this branch as a baseline (as it was already tested to work) and work the newer simplifications on top. |
|
I wrote this reply before @stavros11's one, but I forgot to send before
We agree on the problems and the solution. The further bits are only the requests to:
|
|
I hope this problem is really a very temporary one: as soon as we stabilize the platform creation, we should be able to freely rework the internals without breaking the interface (or, at least, we can schedule the interface breaking, and make it as unfrequent as possible). |
This modifications have reproduced the basic calibration routine but further testing is needed
To do:
Checklist: