Group common Qblox modules functions #730
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
==========================================
+ Coverage 64.00% 64.45% +0.45%
==========================================
Files 49 49
Lines 5778 5700 -78
==========================================
- Hits 3698 3674 -24
+ Misses 2080 2026 -54
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This is also outdated wrt Otherwise, make sure that the branch is updated, and you'd like to merge (I'm asking just because it's not very recent). |
|
@alecandido I put us as reviewers because I think in the last Qibo meeting @PiergiorgioButtarini said that this is ready to merge and overall is reducing the lines of code so would be good to merge. But indeed let's wait for him to fix the conflict and let us know. |
|
Yes, I got your point, but there are a few PRs that are aging a bit in Qibolab, so we should be slightly more careful when merging. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
==========================================
+ Coverage 63.89% 64.54% +0.64%
==========================================
Files 49 49
Lines 5792 5697 -95
==========================================
- Hits 3701 3677 -24
+ Misses 2091 2020 -71
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.
Thanks @PiergiorgioButtarini. I am not sure if there is any qblox available right now and where is the runcard, but if you have access it would be good if you can check that at least instrument tests pass before merging.
| self._sequencers[port] = [] | ||
| if self.settings[port]["lo_frequency"]: | ||
| self._ports[port].lo_enabled = True | ||
| self._ports[port].lo_frequency = self.settings[port]["lo_frequency"] | ||
| self._ports[port].attenuation = self.settings[port]["attenuation"] | ||
| self._ports[port].hardware_mod_en = True | ||
| self._ports[port].nco_freq = 0 | ||
| self._ports[port].nco_phase_offs = 0 |
There was a problem hiding this comment.
I believe this part could also be lifted (if I am not missing something). For example, you could have a _setup_output_port(self, port) in ClusterModule and call it here and for "o1" in QRM.
But I think it is minor, it is still fine as it is.
There was a problem hiding this comment.
Yes you are probably right, I will have a look. Thanks @stavros11
alecandido
added
the
deploy on hardware
|
Superseded by #868 and the subsequent rewriting |
This PR follows the philosophy established in #686 where common properties between the three diferent Qblox modules are defined directly in the class
ClusterModule(inmodule.py) and then are inherited by all of them.TODO:
Test on hardware.
Checklist: