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

David/qubit tracking #581

Merged
merged 15 commits into from
Sep 27, 2023
Merged

David/qubit tracking #581

merged 15 commits into from
Sep 27, 2023

Conversation

DavidSarlle
Copy link
Contributor

@DavidSarlle DavidSarlle commented Sep 14, 2023

This PR adds the parameters need during the qubit flux tracking implemented in qibocal (https://github.com/qiboteam/qibocal/tree/david/qubit_flux_track).

The PR is compatible with all the runcards available in main and qibocal. So we can proceed to merge as soon as the reviewers accept it.

Checklist:

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

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
src/qibolab/qubits.py 93.24% <100.00%> (+0.93%) ⬆️

📢 Thoughts on this report? Let us know!.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks @DavidSarlle. I merged main here because some changes that are already main were appearing in the changes.

It should be fine to merge this now, but we should keep in mind that the update method will be removed when qiboteam/qibocal#526 is merged. @andrea-pasquale please coordinate with @DavidSarlle to properly migrate these updates to the corresponing routines in qibocal.

src/qibolab/qubits.py Outdated Show resolved Hide resolved
src/qibolab/qubits.py Outdated Show resolved Hide resolved
@andrea-pasquale
Copy link
Contributor

It should be fine to merge this now, but we should keep in mind that the update method will be removed when qiboteam/qibocal#526 is merged. @andrea-pasquale please coordinate with @DavidSarlle to properly migrate these updates to the corresponing routines in qibocal.

I will review soon qiboteam/qibocal#519 and I will eventually open a PR on top of that one to fix the update.

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 @DavidSarlle, the code looks good to me.
I will just remove a few lines to avoid creating conflicts with qiboteam/qibocal#526 and #596. I will port those lines in qibocal.

src/qibolab/qubits.py Outdated Show resolved Hide resolved
src/qibolab/platform.py Outdated Show resolved Hide resolved
@stavros11 stavros11 mentioned this pull request Sep 26, 2023
4 tasks
sweetspot: float = 0.0
flux_to_bias: float = 0.0
asymmetry: float = 0.0
brf: float = 0.0
Copy link
Member

Choose a reason for hiding this comment

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

@DavidSarlle what is the final decision for this? Here we are still using brf but in qiboteam/qibolab_platforms_qrc#54 you changed to bare_resonator_frequency_sweetspot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on this I will also update qiboteam/qibocal#538
Personally I think that we should use bare_resonator_frequency_sweetspot.
Also ssf_brf could be renamed to something like ratio_sweetspot_qubit_freq_bare_resonator_freq which is pretty long but at least we know what it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @stavros11 and @andrea-pasquale I forgot to push the latest changes where I modified the brf name to bare_resonator_frequency_sweetspot.

@andrea-pasquale regarding the ssf_brf, we can modifiy it as you said, but is a very large name for a variable... I have also included the docstrings with the explanation, so I think is better if we keep it as it is...

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I will not argue for the name of a single variable.
Let's keep ssf_brf and merge this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Great, if everyone agrees, please merge this when the CI passes.

@andrea-pasquale andrea-pasquale merged commit 2185079 into main Sep 27, 2023
21 checks passed
@andrea-pasquale andrea-pasquale deleted the david/qubit_tracking branch September 27, 2023 10:20
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.

4 participants