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

Add sweeper types #369

Merged
merged 5 commits into from
Jun 5, 2023
Merged

Add sweeper types #369

merged 5 commits into from
Jun 5, 2023

Conversation

rodolfocarobene
Copy link
Contributor

Sweepers type (factor, offset, absolute) added as per qibolab #439.
For now, I tried to not change the expected behaviour of the routines, in particular:

Sweeper Parameter Sweeper Type
Frequency OFFSET
Amplitude FACTOR
Bias ABSOLUTE
Attenuation ABSOLUTE

For attenuation sweepers I'm not sure if absolute is the expected behaviour, I never used them

Checklist:

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

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 @rodolfocarobene, LGTM.

I can confirm you that attenuation is swept using absolute values.
For the future we could also think about passing this sweeper type directly as an argument of the protocol to give more freedom to the user as we were discussing yesterday.

Btw, if you want to see if tests are passing here you can change the qibolab branch in the pyproject.toml and then regenerate the new lock file using poetry lock.

@Edoardo-Pedicillo
Copy link
Contributor

I dismissed the review because the tests are not passing

@andrea-pasquale
Copy link
Contributor

I dismissed the review because the tests are not passing

I think that this is expected since the current lock file is using qibolab main where sweeper type is not implemented.
But you are right, lets wait for tests before approving.

@andrea-pasquale andrea-pasquale dismissed their stale review May 30, 2023 14:28

Waiting for tests

@rodolfocarobene rodolfocarobene added the do not merge Soft warning to avoid merging a PR for reason provided in the PR label May 30, 2023
@rodolfocarobene
Copy link
Contributor Author

Locally everything is passing, but before merging qibolab needs to be updated so this can wait a bit more.
It's not in draft mode since it should not require modifcations (other then maybe the updated poetry lock)

@rodolfocarobene rodolfocarobene removed the do not merge Soft warning to avoid merging a PR for reason provided in the PR label Jun 5, 2023
@rodolfocarobene
Copy link
Contributor Author

@andrea-pasquale @Edoardo-Pedicillo, this PR should be know ready for review and mergable (since we merged the qibolab PR).

Here I broke almost all the niGSC tests when updating the lock file (in particular using latest version of qibo 0.0.14)...
The problem is fixed when fixing in the lock file qibo==0.0.13, but it should be addressed in some other way.

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #369 (0a7c097) into main (5d5b713) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
- Coverage   54.12%   54.11%   -0.01%     
==========================================
  Files          68       68              
  Lines        5367     5366       -1     
==========================================
- Hits         2905     2904       -1     
  Misses       2462     2462              
Flag Coverage Δ
unittests 54.11% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...qibocal/protocols/characterization/coherence/t1.py 100.00% <ø> (ø)
...cal/protocols/characterization/dispersive_shift.py 100.00% <100.00%> (ø)
...terization/flux_depedence/qubit_flux_dependence.py 100.00% <100.00%> (ø)
...zation/flux_depedence/resonator_flux_dependence.py 100.00% <100.00%> (ø)
...l/protocols/characterization/qubit_spectroscopy.py 100.00% <100.00%> (ø)
...bocal/protocols/characterization/rabi/amplitude.py 65.59% <100.00%> (ø)
...l/protocols/characterization/resonator_punchout.py 95.90% <100.00%> (ø)
...characterization/resonator_punchout_attenuation.py 93.46% <100.00%> (ø)
...otocols/characterization/resonator_spectroscopy.py 100.00% <100.00%> (ø)
...acterization/resonator_spectroscopy_attenuation.py 100.00% <100.00%> (ø)

@Edoardo-Pedicillo
Copy link
Contributor

The issue with niGSC tests is solved in #380, that was merged in main. Merging the main in your branch should solve the problem.

@rodolfocarobene
Copy link
Contributor Author

The issue with niGSC tests is solved in #380, that was merged in main. Merging the main in your branch should solve the problem.

You are right, I thought to have merged main but I forgot to pull... sorry!

@andrea-pasquale andrea-pasquale merged commit a596e04 into main Jun 5, 2023
@andrea-pasquale andrea-pasquale deleted the sweeper-types branch June 5, 2023 13:19
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.

3 participants