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

Minor adjustments in result objects #896

Merged
merged 6 commits into from
May 14, 2024
Merged

Conversation

andrea-pasquale
Copy link
Contributor

@andrea-pasquale andrea-pasquale commented May 8, 2024

Required by qiboteam/qibocal#832
This PR "corrects" the average property of AveragedIntegratedResults by returning the object itself without performing the average again.
I've also added the property phase_std which is useful for qibocal (if you think that it is not necessary I can always drop it and do it directly in qibocal).
I'm keeping it as draft to account for any possible changes in qiboteam/qibocal#832

EDIT: I've also implemented #897 here.
Checklist:

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

@andrea-pasquale andrea-pasquale marked this pull request as draft May 8, 2024 06:15
Copy link

codecov bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 66.62%. Comparing base (41fcafd) to head (e1a7f84).
Report is 28 commits behind head on main.

Files Patch % Lines
src/qibolab/result.py 78.57% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
+ Coverage   66.61%   66.62%   +0.01%     
==========================================
  Files          55       55              
  Lines        5942     5954      +12     
==========================================
+ Hits         3958     3967       +9     
- Misses       1984     1987       +3     
Flag Coverage Δ
unittests 66.62% <78.57%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrea-pasquale andrea-pasquale marked this pull request as ready for review May 8, 2024 14:17
@andrea-pasquale andrea-pasquale self-assigned this May 8, 2024
Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

Fine for 0.1, I'd replace all results objects in 0.2 with arrays (possibly newtype or annotated for the sake of the type check, but just arrays at runtime).

I just left a few minor comments, but they are not changing anything drastically.

@property
def average(self):
"""Perform average over i and q."""
return self
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an error?

(It's true that is already averaged, but asking for its average doesn't make sense)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a temporary patch for qibocal.

@cached_property
def phase_std(self):
"""Signal phase in radians."""
return None
Copy link
Member

Choose a reason for hiding this comment

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

Even this one sounds more appropriate as an error...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

Co-authored-by: Alessandro Candido <[email protected]>
@andrea-pasquale andrea-pasquale requested a review from hay-k May 14, 2024 07:57
Copy link
Contributor

@hay-k hay-k left a comment

Choose a reason for hiding this comment

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

I think for now this is fine as a hack to support qibocal needs. In long term I agree that these result classes should be revisited and simplified.

@alecandido
Copy link
Member

In long term I agree that these result classes should be revisited and simplified.

Since it's only listed in the events, but not linked in any comment, let me note explicitly that it has already been scheduled for 0.2 #899

I could provide a PR with the *Results removal, and basic replacement in the common layer. However, replacing with just arrays means that most of the work will have to be done in the drivers (just returning arrays instead of *Result) and Qibocal (handling it properly).

@andrea-pasquale andrea-pasquale merged commit b93ee1b into main May 14, 2024
24 checks passed
@stavros11 stavros11 deleted the fix_result_average branch May 14, 2024 11:19
@andrea-pasquale andrea-pasquale mentioned this pull request May 31, 2024
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