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

Remove qualang-tools dependency and QM simulation #1131

Merged
merged 4 commits into from
Jan 17, 2025
Merged

Conversation

stavros11
Copy link
Member

The price to pay is that we need to grab the qualang_tools functions we are using and put them in our code but given that they are quite simple, I find it preferable over having many dependencies (dash, Flask, etc.) which do not make much sense for qibolab as a driver and not reporting library.

I also decided to drop the simulation capability from the QM driver, as it was quite useless as it was and to avoid porting one more qualang_tools function. Since we need to write custom code to process the simulation output anyway, it is usually easier to just get the QUA script we are interested in simulating and doing the simulation externally, without qibolab.

I still need to confirm these changes don't break anything.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 0% with 63 lines in your changes missing coverage. Please review.

Project coverage is 50.74%. Comparing base (01f1886) to head (dd403af).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...bolab/_core/instruments/qm/program/instructions.py 0.00% 51 Missing ⚠️
...ibolab/_core/instruments/qm/program/acquisition.py 0.00% 9 Missing ⚠️
src/qibolab/_core/instruments/qm/controller.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1131      +/-   ##
==========================================
- Coverage   51.34%   50.74%   -0.60%     
==========================================
  Files          63       63              
  Lines        2984     3019      +35     
==========================================
  Hits         1532     1532              
- Misses       1452     1487      +35     
Flag Coverage Δ
unittests 50.74% <0.00%> (-0.60%) ⬇️

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.

@stavros11
Copy link
Member Author

I tested several routines on qw11q using this branch and all seems fine, so I will mark as ready for review.

@stavros11 stavros11 marked this pull request as ready for review January 17, 2025 11:58
@stavros11 stavros11 requested a review from alecandido January 17, 2025 11:58
@alecandido alecandido changed the title Remove qualang-tools dependecy Remove qualang-tools dependency and QM simulation Jan 17, 2025
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.

I didn't read the whole of _qua_for_loop, but I guess you mostly copied it from qualang_tools.
Maybe, since we should (at some point), since from now on we'll maintain that code.

But for the time being, I'm fine with the idea, and we can merge this.

Thanks!

src/qibolab/_core/instruments/qm/program/instructions.py Outdated Show resolved Hide resolved
)

# Logarithmic increment
if np.isclose(np.std(array[1:] / array[:-1]), 0, atol=1e-3):
Copy link
Member

Choose a reason for hiding this comment

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

Funny condition...

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exact copy from qualang_tools.

Co-authored-by: Alessandro Candido <[email protected]>
@stavros11
Copy link
Member Author

Thanks for the review.

I didn't read the whole of _qua_for_loop, but I guess you mostly copied it from qualang_tools.

Indeed. In terms of behavior, the only change is that the original function in some cases raises an error prompting the user to use for_each_ instead of for_. Here I am actually returning the corresponding for_each_ instead of raising the error, in order to make it slightly more general.

Generally for_ supports only "equidistant" grids in the linear or logarithmic scale, while for_each_ supports arbitrary values. In principle, we could completely avoid this function and always use for_each_, however for_ is expected to be more efficient (and memory friendly) when it is possible to use it (https://docs.quantum-machines.co/latest/docs/Guides/best_practices/?h=for_each_#loops).

@stavros11 stavros11 merged commit e8df7f5 into main Jan 17, 2025
26 of 28 checks passed
@stavros11 stavros11 deleted the rm-qualang-tools branch January 17, 2025 13:17
@alecandido
Copy link
Member

Generally for_ supports only "equidistant" grids in the linear or logarithmic scale, while for_each_ supports arbitrary values. In principle, we could completely avoid this function and always use for_each_, however for_ is expected to be more efficient (and memory friendly) when it is possible to use it (docs.quantum-machines.co/latest/docs/Guides/best_practices?h=for_each_#loops).

@stavros11 well, this is something we should actually investigate deeper (not necessarily immediately).

While for simulation there is a clear answer regarding performances (i.e. the shortest the better), that's not the same for hardware (or in general real-time execution).
In this case, the tradeoff is between flexibility and consistency: we do not want necessarily to take fewer clock cycles (we may even have spare cycles, while reproducing the pulses), but we want the timing to be as predictable as possible.

In any case, let's keep it in mind, and postpone the discussion.

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.

2 participants