Remove offset initialization in QCM_BB

[PiergiorgioButtarini]

Since the offset is already initialized as the qubit sweetspot this setting was removed from the runcard.
This PR introduce a ClusterModule(Instrument) class from which each module inherits, in order to describe some common properties between the three modules.

Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

[codecov]

Codecov Report

Attention: 64 lines in your changes are missing coverage. Please review.

Comparison is base (cc8a1b4) 63.25% compared to head (c455f7e) 63.58%.

Files	Patch %	Lines
src/qibolab/instruments/qblox/cluster_qcm_bb.py	17.85%	23 Missing ⚠️
src/qibolab/instruments/qblox/cluster_qrm_rf.py	8.69%	21 Missing ⚠️
src/qibolab/instruments/qblox/cluster_qcm_rf.py	9.09%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #686      +/-   ##
==========================================
+ Coverage   63.25%   63.58%   +0.33%     
==========================================
  Files          48       49       +1     
  Lines        6596     6539      -57     
==========================================
- Hits         4172     4158      -14     
+ Misses       2424     2381      -43     

Flag	Coverage Δ
unittests	63.58% <29.67%> (+0.33%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stavros11]

@PiergiorgioButtarini to simplify the review, is this PR still draft (meaning you are planning to work on it), ready or superseded by #702? I am asking because it is modifying the cluster which we are then removing, so there may be conflicts depending on how we merge these.

[PiergiorgioButtarini]

Is it still draft, me and @alecandido will work on it in the next days. So maybe it will be useful if in the meantime we can merge #702.

[alecandido]

@stavros11 I would consider this at an early stage, while #702 is complete or close to completion. If there is any conflict, it will be fixed in here.

[PiergiorgioButtarini]

After the latest commits (especially b7a1efc) I would consider this as ready for review.
I think it will be useful to merge this since I'm using this branch for other implementations like updating the docstrings and adding other methods/properties in the module.py (but I don't want to increase the size of these PR).

[stavros11]

Thanks @PiergiorgioButtarini, looks good to me and it works, only a minor comment below.
One issue before merging: did you try to run pytest on qpu? I get the following error:

@pytest.fixture(scope="module")
  def connected_qcm_rf(connected_controller, connected_cluster):
E       fixture 'connected_cluster' not found

maybe this fixture is not properly defined? The errors are only in the qblox tests, the platform and backend tests are working.

[alecandido]

@PiergiorgioButtarini before merging, it would be useful to also have a working runcard for this PR and testing it on hardware.

E.g. the .port() method is new, and has never been tested (and a test w/o hardware would not be fully reliable - especially because we don't have yet a suitable infrastructure for them).

[stavros11]

@PiergiorgioButtarini before merging, it would be useful to also have a working runcard for this PR and testing it on hardware.

I used the qw5q_gold platform from the remove_offset branch of qibolab_platforms_qrc to test this on hardware. QPU tests are failing with the error I wrote above but other than that, it seems to work.

[PiergiorgioButtarini]

@PiergiorgioButtarini before merging, it would be useful to also have a working runcard for this PR and testing it on hardware.

E.g. the .port() method is new, and has never been tested (and a test w/o hardware would not be fully reliable - especially because we don't have yet a suitable infrastructure for them).

Thanks @alecandido, as Stavros said there are already runcards to test it.
I link the report that I just made with the latest version of this PR and the latest remove_offset branch of platform_qrc:
http://login.qrccluster.com:9000/pXlhiY-8SC2_mbpTjkumwg==

[PiergiorgioButtarini]

Thanks @PiergiorgioButtarini, looks good to me and it works, only a minor comment below. One issue before merging: did you try to run pytest on qpu? I get the following error:

@pytest.fixture(scope="module")
  def connected_qcm_rf(connected_controller, connected_cluster):
E       fixture 'connected_cluster' not found

maybe this fixture is not properly defined? The errors are only in the qblox tests, the platform and backend tests are working.

This is because now the cluster object doesn't exist anymore. I'll fix the tests asap, thanks for spotting it.

[stavros11]

Thank you for the updates, tests are now passing.

[PiergiorgioButtarini]

Thanks for the review @alecandido , I implemented your suggestions in the last commit.

[stavros11]

@PiergiorgioButtarini thanks for the updates, could you also please fix the conflict?

[alecandido]

Thanks @PiergiorgioButtarini, we can merge.

When you apply the comments (or solve them anyhow) close the corresponding conversation, to better keep track of things that are still opened (and, if possible, just add a final comment with the hash of the commit solving it).

[PiergiorgioButtarini]

Thanks @PiergiorgioButtarini, we can merge.

When you apply the comments (or solve them anyhow) close the corresponding conversation, to better keep track of things that are still opened (and, if possible, just add a final comment with the hash of the commit solving it).

Thanks for the suggestion, I'll be more organized next time and I will follow this tips!

[stavros11]

@PiergiorgioButtarini also after merging this, please remember to update the platforms in the qrc repository for the API changes so that they work when we cool down.