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

Removing precision sweep #270

Merged
merged 9 commits into from
Mar 20, 2023
Merged

Removing precision sweep #270

merged 9 commits into from
Mar 20, 2023

Conversation

andrea-pasquale
Copy link
Contributor

@andrea-pasquale andrea-pasquale commented Mar 8, 2023

Closes #268.

Checklist:

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

@andrea-pasquale andrea-pasquale self-assigned this Mar 8, 2023
@andrea-pasquale andrea-pasquale added the enhancement New feature or request label Mar 8, 2023
@andrea-pasquale andrea-pasquale added this to the Qibocal 0.0.2 milestone Mar 8, 2023
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #270 (9e7ad40) into main (bb98e7f) will increase coverage by 0.14%.
The diff coverage is 5.26%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #270      +/-   ##
==========================================
+ Coverage   44.02%   44.17%   +0.14%     
==========================================
  Files          28       28              
  Lines        1774     1768       -6     
==========================================
  Hits          781      781              
+ Misses        993      987       -6     
Flag Coverage Δ
unittests 44.17% <5.26%> (+0.14%) ⬆️

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

Impacted Files Coverage Δ
src/qibocal/plots/spectroscopies.py 4.81% <0.00%> (+0.09%) ⬆️
src/qibocal/fitting/methods.py 71.42% <100.00%> (ø)

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.

Thank you for implementing this. As I wrote in #268 I agree with this change but I am not sure if this precision sweep has some use for others.

Comment on lines 16 to 17
width: int,
step: int,
Copy link
Member

Choose a reason for hiding this comment

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

That is fine with me, but maybe something like

Suggested change
width: int,
step: int,
frequency_width: int,
frequency_step: int,

would be clearer, so that we know what we sweep?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe freq_width so that it is in agreement with resonator_punchout etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I think that freq_width is fine.

@andrea-pasquale andrea-pasquale merged commit c907c6a into main Mar 20, 2023
@andrea-pasquale andrea-pasquale deleted the remove_sweep branch March 20, 2023 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remove precision sweep
3 participants