qiboteam / qibolab

Quantum hardware module and drivers for Qibo.
https://qibo.science
Apache License 2.0
41 stars 12 forks source link

Drop symbolic expressions #744

Closed alecandido closed 8 months ago

alecandido commented 8 months ago

I thought I dropped symbolic.py in #709, but I was wrong (sorry @hay-k). Here I am doing what I forgot to do there (since all the references were already removed in that PR).

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0765763) 62.94% compared to head (123e154) 62.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #744 +/- ## ========================================== - Coverage 62.94% 62.00% -0.95% ========================================== Files 49 48 -1 Lines 6399 6095 -304 ========================================== - Hits 4028 3779 -249 + Misses 2371 2316 -55 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/744/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibolab/pull/744/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `62.00% <ø> (-0.95%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alecandido commented 8 months ago

My bad, I forgot to grep case-insensitive.

However, I'm surprised that everything is passing, since there are still a bunch of mentions I forgot:

❯ rg '\bse_'
src/qibolab/pulses.py
1026:        # def __init__(self, start:int | se_int, duration:int | se_int, amplitude:float, frequency:int, relative_phase:float, shape: PulseShape | str,
1083:        # def __init__(self, start:int | se_int, duration:int | se_int, amplitude:float, frequency:int, relative_phase:float, shape: PulseShape | str,
1113:        # def __init__(self, start:int | se_int, duration:int | se_int, amplitude:float, frequency:int, relative_phase:float, shape: PulseShape | str,

tests/test_instruments_zhinst.py
379:            duration=ro_pulses[qubit].se_start,

tests/test_instruments_qmsim.py
443:            start=y90_pulse.se_finish,
451:            lowfreq, start=flux_pulse.se_finish, relative_phase=np.pi / 3
454:            highfreq, start=flux_pulse.se_finish, relative_phase=0
458:            lowfreq, start=y90_pulse.se_finish, relative_phase=np.pi / 3
461:            highfreq, start=x_pulse_start.se_finish, relative_phase=0
465:        lowfreq, start=theta_pulse.se_finish
468:        highfreq, start=x_pulse_end.se_finish
stavros11 commented 8 months ago

However, I'm surprised that everything is passing, since there are still a bunch of mentions I forgot:

From qmsim tests, they require specific options to be passed in the pytest command so they are not executed in the CI. Btw, I'm planning to drop these tests (possibly in one of the QM PRs).

For the Zurich test, it's a bit weird that it didn't fail. It is not marked as qpu so it should not be skipped. Maybe that's worth investigating.

alecandido commented 8 months ago

That test is actually skipped, I just checked. I'm trying to understand why.

EDIT: it is skipped in the sense that it is not executed (I put a pdb inside the function), but reports it as PASSED

tests/test_instruments_zhinst.py::test_experiment_execute_pulse_sequence_coupler PASSED          [100%]

so it's not really skipped...

RE-EDIT: it was the wrong one, I didn't notice the _coupler suffix

alecandido commented 8 months ago

I found it: https://github.com/qiboteam/qibolab/blob/0765763ffc14feef40fbdb6a726bf1b24f594fba/tests/test_instruments_zhinst.py#L231 https://github.com/qiboteam/qibolab/blob/0765763ffc14feef40fbdb6a726bf1b24f594fba/tests/test_instruments_zhinst.py#L355 https://github.com/qiboteam/qibolab/blob/0765763ffc14feef40fbdb6a726bf1b24f594fba/tests/test_instruments_zhinst.py#L764

It's overwritten, and the last one is actually marked as qpu

alecandido commented 8 months ago

I'd rather avoid fixing that problem in this PR, since it's not related to symbolic at all. If you agree @stavros11, I'd just merge (feel free to do it yourself).