Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix frequency range type #803

Merged
merged 1 commit into from
Apr 19, 2024
Merged

fix frequency range type #803

merged 1 commit into from
Apr 19, 2024

Conversation

Edoardo-Pedicillo
Copy link
Contributor

This PR closes #762

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.
  • Compatibility with Qibo modules (Please edit this section if the current pull request is not compatible with the following branches).
    • Qibo: master
    • Qibolab: main
    • Qibolab_platforms_qrc: main

@Edoardo-Pedicillo Edoardo-Pedicillo changed the title fix: avoid rounding frequency intervals fix frequency range type Apr 18, 2024
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.11%. Comparing base (7c9eee7) to head (a733079).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #803      +/-   ##
==========================================
- Coverage   97.13%   97.11%   -0.03%     
==========================================
  Files         104      104              
  Lines        7786     7786              
==========================================
- Hits         7563     7561       -2     
- Misses        223      225       +2     
Flag Coverage Δ
unittests 97.11% <100.00%> (-0.03%) ⬇️

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

Files Coverage Δ
.../protocols/characterization/coherence/spin_echo.py 95.12% <100.00%> (ø)
...ols/characterization/coherence/spin_echo_signal.py 100.00% <100.00%> (ø)
...cterization/couplers/coupler_qubit_spectroscopy.py 100.00% <ø> (ø)
...ization/couplers/coupler_resonator_spectroscopy.py 100.00% <ø> (ø)
...cal/protocols/characterization/dispersive_shift.py 100.00% <ø> (ø)
...tocols/characterization/dispersive_shift_qutrit.py 100.00% <ø> (ø)
...haracterization/flux_dependence/qubit_crosstalk.py 93.12% <ø> (ø)
...erization/flux_dependence/qubit_flux_dependence.py 97.63% <ø> (ø)
...cterization/flux_dependence/qubit_flux_tracking.py 89.06% <ø> (ø)
...cterization/flux_dependence/resonator_crosstalk.py 95.03% <ø> (ø)
... and 6 more

... and 1 file with indirect coverage changes

Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Edoardo-Pedicillo.
Two points:

  • Can we have a function to generate the delta_frequency_range? In this way we can avoid to change many times the same thing.
  • Before merging we should check that all drivers work with float frequencies? Is this the case @stavros11 @hay-k @alecandido?

@alecandido
Copy link
Member

Before merging we should check that all drivers work with float frequencies? Is this the case @stavros11 @hay-k @alecandido?

This was one (tiny) point in Qibolab 0.2: the goal is to always make use of floats in the interface for real quantities (not only for frequencies), and integers inside the drivers (when needed).

I would say that there is no electronics actually supporting floats, but they should all support fixed precision. So, the conversion should happen inside.
However, I still have it on my list, and the final proof is only to see them working, of course...

(the rfsoc is already using floats for frequencies, laboneq should always accept floats, and in QUA you declare them as fixed, but they are initialized with floats - for qblox I still have to find out...)

https://github.com/qiboteam/qibolab/blob/a514441f7c020b190c7871df90d6892392d0b4da/src/qibolab/instruments/rfsoc/convert.py#L117

@stavros11
Copy link
Member

stavros11 commented Apr 18, 2024

In QM frequency sweeps are casted to int by the driver (https://github.com/qiboteam/qibolab/blob/a514441f7c020b190c7871df90d6892392d0b4da/src/qibolab/instruments/qm/sweepers.py#L97) so this change shouldn't affect. I also checked the resonator spectroscopy and it works.

and in QUA you declare them as fixed, but they are initialized with floats

That's correct, however for frequency sweeps which are relevant for this PR we have to use the update_frequency function which updates frequencies in real time, so it also matters what type this accepts. From their docs this is not clear, since new_frequency is of type int but in the example they pass floats... It seems to also support units, so most likely float is fine. However, sub-Hz resolution (if we ever need that), seems to need special treatment (see example).

In any case, I agree that these conversions should be handled by the drivers and we should just use floats externally.

@andrea-pasquale
Copy link
Contributor

Thanks for the feedback @stavros11 @alecandido!
After testing we should be able to merge the PR, then.
If we see that something is not working we should wait for qibolab 0.2, correct?

@Edoardo-Pedicillo
Copy link
Contributor Author

The changes do not brake QM
http://login.qrccluster.com:9000/Rke3NbOaRCKA_KK7EIvyKg==/

@alecandido
Copy link
Member

If we see that something is not working we should wait for qibolab 0.2, correct?

The moment we will get to the drivers' update, the goal is to make the required casts in the driver (this will go together a units update, since the goal is to use GHz and ns everywhere, and to avoid 1e9/1e6 conversions - confining all the required conversions to the drivers).

@andrea-pasquale
Copy link
Contributor

Also Zurich seems fine http://login.qrccluster.com:9000/vwrYTwXoTGe6Nu8u1KYkuQ==/

@andrea-pasquale
Copy link
Contributor

andrea-pasquale commented Apr 18, 2024

@andrea-pasquale andrea-pasquale added this pull request to the merge queue Apr 19, 2024
Merged via the queue into main with commit fbdade2 Apr 19, 2024
21 checks passed
@andrea-pasquale andrea-pasquale deleted the frequency_range branch April 19, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frequency range type
5 participants