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

Gate set tomography #1106

Merged
merged 287 commits into from
Jul 20, 2024
Merged

Gate set tomography #1106

merged 287 commits into from
Jul 20, 2024

Conversation

mho291
Copy link
Contributor

@mho291 mho291 commented Nov 24, 2023

One qubit calibration (empty circuit tomography)
Two qubit calibration (empty circuit tomography)
One qubit operator(s) gate set tomography
Two qubits operator(s) gate set tomography

Checklist:

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

@mho291 mho291 assigned mho291 and unassigned mho291 Nov 24, 2023
@mho291 mho291 changed the title Added gate set tomography file Gate set tomography Nov 24, 2023
@scarrazza scarrazza added this to the Qibo 0.2.5 milestone Nov 29, 2023
@MatteoRobbiati MatteoRobbiati marked this pull request as ready for review November 29, 2023 09:09
Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi left a comment

Choose a reason for hiding this comment

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

Thanks @mho291 for implementing this. I left some comments below, I'm not finished yet, but in general I have the feeling that many code repetitions can be avoided. I would try to write some general functions that can be applied to all the different cases you encounter here. This would make the code much easier to follow and clean.

src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
@mho291
Copy link
Contributor Author

mho291 commented Dec 6, 2023

Hi @BrunoLiegiBastonLiegi! Most of the issues you've raised have been resolved. Thanks so much for looking through the code and providing helpful comments. The code that I have in my local device has all the resolved issues implemented and it works. Perhaps we can discuss some of the outstanding issues before I do another push. Thanks a lot!

@BrunoLiegiBastonLiegi
Copy link
Contributor

Hi, thanks for addressing the comments, can you push your local changes such that I can give it a look please?

Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @mho291. I think that some improvements are possible for the GST_measure_* functions. I still have to go over the rest but I would focus on those first.

src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi left a comment

Choose a reason for hiding this comment

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

Thanks @mho291 for the updates. I made some further comments. In general I have the impression that this could be made a lot shorter by applying smarter design choices. Avoid hard coded definitions when possible, and try to write general helper functions that can be specialized for the different use cases. This would also make the code much easier to follow. Moreover, if there is a reference available for this implementation, please attach it to the first post in this PR.

src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
@AlejandroSopena
Copy link
Contributor

Hi @mho291, can you push your latest changes so we can continue reviewing from there please?

@mho291
Copy link
Contributor Author

mho291 commented Dec 19, 2023

Hi @mho291, can you push your latest changes so we can continue reviewing from there please?

Just did. Sorry for the delay. I planned to push yesterday but I caught the covid bug.

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (465e88d) 100.00% compared to head (2592ec8) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1106   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           69        71    +2     
  Lines        10110     10202   +92     
=========================================
+ Hits         10110     10202   +92     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

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.

Copy link
Contributor

@AlejandroSopena AlejandroSopena left a comment

Choose a reason for hiding this comment

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

Thanks. The implementation of the method seems correct. Just a few comments and suggestions to shorten the code and avoid redundancies.

src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
Copy link
Contributor

@AlejandroSopena AlejandroSopena left a comment

Choose a reason for hiding this comment

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

Hi @mho291. Thanks for the changes. Here are some new suggestions:

src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
src/qibo/validation/gate_set_tomography.py Outdated Show resolved Hide resolved
@renatomello
Copy link
Contributor

renatomello commented Jan 19, 2024

@scarrazza @andrea-pasquale @Jacfomg Following conversations with @AlejandroSopena @ QIP 2024, I'd like to propose that the validation module created here be renamed to tomography and we let this be the initial PR of a new module that would serve both for simulation of tomography (important and necessary on its own and will be necessary for future projects) and also as primitives to qibocal (e.g. https://github.com/qiboteam/qibocal/tree/main/src/qibocal/protocols/characterization/randomized_benchmarking).

@scarrazza
Copy link
Member

Sure, the renaming makes sense.

mho291 and others added 3 commits January 24, 2024 10:43
- Added a comment on L93 to discuss.
- Restructured the conditional statements in GST_execute_circuit().
@renatomello renatomello self-requested a review January 24, 2024 03:31
@renatomello renatomello added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 24, 2024
@renatomello renatomello added this pull request to the merge queue Jul 20, 2024
Merged via the queue into master with commit ae403e2 Jul 20, 2024
27 checks passed
@renatomello renatomello deleted the Gate-set-tomography branch July 20, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants