qilimanjaro-tech / qililab

Qililab is a generic and scalable quantum control library used for fast characterization and calibration of quantum chips. Qililab also offers the ability to execute high-level quantum algorithms with your quantum hardware.
Apache License 2.0
32 stars 3 forks source link

[QHC-636] Create AnnealingSchedule class in Qililab. #767

Closed visagim closed 2 months ago

linear[bot] commented 3 months ago

QHC-636 Create AnnealingSchedule class in Qililab.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.81%. Comparing base (ed5a9f5) to head (54277d8). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #767 +/- ## ========================================== + Coverage 95.77% 95.81% +0.03% ========================================== Files 266 267 +1 Lines 8593 8670 +77 ========================================== + Hits 8230 8307 +77 Misses 363 363 ``` | [Flag](https://app.codecov.io/gh/qilimanjaro-tech/qililab/pull/767/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qilimanjaro-tech) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qilimanjaro-tech/qililab/pull/767/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qilimanjaro-tech) | `95.81% <100.00%> (+0.03%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qilimanjaro-tech#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

visagim commented 2 months ago

Added a new comment and there still one unresolved question from previous review and is good to go 💯 🚀

what's the unresolved comment?

fedonman commented 2 months ago

Added a new comment and there still one unresolved question from previous review and is good to go 💯 🚀

what's the unresolved comment?

this, you can find it if you scroll higher.

image

visagim commented 2 months ago

Also a further note on this

Also, why have we chosen the transpiler to be a Callable and not a class? Now the method transpile() belongs to AnnealingProgram when it should belong to the Transpiler class.

The flux to ising transpiler is very much at its infancy still, and there is a lot of uncertainty about how will it evolve further than the one qubit case. That is why I am reluctant about starting to make classes and big structures for a workflow that is sure to change in the short term (1-2months) - I think it's better to keep something minimal and make it more rigid (efficient) as we consolidate each step and are sure that that's how it's going to be in the mid term (5-10months). But I do believe at some point we'll definitely make a transpiler class with more complicated logic, just think that right now it's overkill

fedonman commented 2 months ago

Also a further note on this

Also, why have we chosen the transpiler to be a Callable and not a class? Now the method transpile() belongs to AnnealingProgram when it should belong to the Transpiler class.

The flux to ising transpiler is very much at its infancy still, and there is a lot of uncertainty about how will it evolve further than the one qubit case. That is why I am reluctant about starting to make classes and big structures for a workflow that is sure to change in the short term (1-2months) - I think it's better to keep something minimal and make it more rigid (efficient) as we consolidate each step and are sure that that's how it's going to be in the mid term (5-10months). But I do believe at some point we'll definitely make a transpiler class with more complicated logic, just think that right now it's overkill

It's not an overkill if we know that we will need it eventually. Making the change after 2+ months of additional development will be much more difficult than it is to make it now.

fedonman commented 2 months ago

Also a further note on this

Also, why have we chosen the transpiler to be a Callable and not a class? Now the method transpile() belongs to AnnealingProgram when it should belong to the Transpiler class.

The flux to ising transpiler is very much at its infancy still, and there is a lot of uncertainty about how will it evolve further than the one qubit case. That is why I am reluctant about starting to make classes and big structures for a workflow that is sure to change in the short term (1-2months) - I think it's better to keep something minimal and make it more rigid (efficient) as we consolidate each step and are sure that that's how it's going to be in the mid term (5-10months). But I do believe at some point we'll definitely make a transpiler class with more complicated logic, just think that right now it's overkill

It's not an overkill if we know that we will need it eventually. Making the change after 2+ months of additional development will be much more difficult than it is to make it now.

My main concern is that this is public API since it is in platform.execute_annealing() and it will be much difficult to change the API once we start using it.

If you answer the other question above, How do we expect users to create/use these transpilers? Do we have a number of predefined algorithms for transpilation?, we can try to find an minimal OOP design that provides a friendly UX API.

My current guess is an AnnealingTranspiler generic class, that can be sub-classed to provide different pre-defined algorithms, or also initialized using a custom Callable as it is now.

That way the signature of platform.execute_annealing() would be (annealing_program: AnnealingProgram, transpiler: AnnealingTranspiler) which makes it more UX friendly than (anneal_program_dict: list[dict[str, dict[str, float]]], transpiler: Callable)

Very good points, as to how is it gonna be long term, also think we are aiming to be a company that makes custom chips, that is, analog chips that solve particular problems, so this needs to be also in our minds, how to make this as generic as possible to fit within that technical development.

visagim commented 2 months ago

I added a ticket to address (I believe) all of the suggestions above and marked related conversations as resolved (as I believe they have been). Once @fedonman gives the OK I'll merge