Closed andrea-pasquale closed 3 months ago
codecov[bot]
commented
4 months ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 97.32%. Comparing base (
a1161f6) to head (7a03b1b).:exclamation: Current head 7a03b1b differs from pull request most recent head c10d86d
Please upload reports for the commit c10d86d to get more accurate results.
alecandido
commented
4 months ago To make it (slightly) simpler, I would:
list[Union[Action, ActionLike]], where ActionLike is a dictionary that could be loaded into an actionplatform a Union[Platform, str] (make the call to create_platform() internally, if it's not already an instance of Platform)output a PathLike (same of the previous)Executor top-level.run_protocol() parameters, to require only the protocol idacquire + fit as default mode
auto as well as a keyword, but I'd avoid generating reports because it would happen during the executionturning your example into:
from qibocal import Executor
protocol = {"id": "flipping",
"operation": "flipping",
"targets": [0,1,2],
"parameters": {
"nflips_max": 20,
"nflips_step": 2,
"detuning": +0.1,
}}
executor = Executor.create([protocol], platform="dummy", output="FOLDER")
completed = executor.run_protocol("flipping", mode="acquire")This is just for interfacing purposes, saving the user some boilerplate and limiting the required understanding of Qibocal internals.
alecandido
commented
4 months ago Concerning the mode, currently, Qibocal is using: https://github.com/qiboteam/qibocal/blob/a668a08c9cfd118f185bcd17ab80acc00fbc27eb/src/qibocal/auto/mode.py#L4-L8
However, we could use an IntFlag and define auto as:
auto = ExecutionMode.acquire | ExecutionMode.fit | ExecutionMode.reportwith all the bits set.
For .run_protocol() I'd just use ExecutionMode.acquire | ExecutionMode.fit. We could give it a name, or, to make it simpler, set the default to None, and set the mode internally to that, instead of retrieving it by name.
alecandido
commented
4 months ago Apart from names and other interface suggestions, the current proposal looks essentially good: it should be minimal, but it should allow running a fully flexible calibration program.
Of course, if you have many of these protocols (calibration programs, not individual routines) there will be repeated patterns (e.g. the dependencies) and we could abstract them in the framework. But given that (for the time being) we do not even have a single one of them, I believe this could be an optimal starting point :)
andrea-pasquale
commented
4 months ago Thanks for the review @alecandido.
I think I have addressed all comments.
There is only one think worth mentioning ExecutionMode.REPORT will not actually generate any report.
It is meant to indicate a mode where both acquisition and fit are skipped. The data is just loaded and then pass it to the ReportBuilder to perform the report generation. At this point we could even drop it or expose to the user a proper way to generate the report.
alecandido
commented
4 months ago At this point we could even drop it or expose to the user a proper way to generate the report.
Yes, that's definitely an option.
Since the Executor is not involved in the report generation, we could restrict the modes to acquire and fit (and the combination), and instead delegate the rest to the ReportBuilder.
This would only change the modes and the way the CLI is implemented, but you could keep the exact same interface with qq auto/acquire/fit/report.
andrea-pasquale
commented
4 months ago The only thing missing is an example of how to use it, but given that is a non-breaking/minimal contribution you could even merge it like this, and contribute the example in a further PR, considering this just as a refactor that will allow that feature (and not a new feature to be documented).
I can write an example on how to use it. We have already a section in documentation on how to use qibocal as a library https://qibo.science/qibocal/stable/tutorials/advanced.html#how-to-use-qibocal-as-a-library, which was doing similar things in a more complicated way. At this point shall we remove it and modify this section with the new layout proposed here? Again the only difference concern the plotting, I believe that we the current layout it is not trivial to generate plots. What do you think?
scarrazza
commented
4 months ago In view of #866, I would suggest a minor API change (demo code):
from qibocal import Executor, Routine
from qibocal.protocols.operation import flipping
flipping_params = {
"id" : "flipping initial guess",
"targets": [0,1,2],
"parameters": {
"nflips_max": 20,
"nflips_step": 2,
"detuning": +0.1,
}}
def acquire(data, ...):
"""Custom routine"""
return ...
my_routine = Routine(acquire)
routine_params = {"......."}
executor = Executor.create(platform="dummy", output="FOLDER")
for i in range(10):
completed = executor.run_protocol(flipping, flipping_params, mode="acquire")
flipping_params['parameters']['detuning'] += i
completed2 = executor.run_protocol(my_routine, routine_params, mode="acquire")Where:
Executor.create the protocols that are already registered internally in qibocal.Executor with custom plugin routines.alecandido
commented
4 months ago We have already a section in documentation on how to use qibocal as a library qibo.science/qibocal/stable/tutorials/advanced.html#how-to-use-qibocal-as-a-library, which was doing similar things in a more complicated way. At this point shall we remove it and modify this section with the new layout proposed here?
Yes, and I'd move it out from "Advanced Examples", and rather make its own page under tutorials (just to avoid passing the idea that this is something you should beware of).
Again the only difference concern the plotting, I believe that we the current layout it is not trivial to generate plots. What do you think?
Currently, we're coupling report generation to the execution, though the two things are well separate.
What we could do is to expose the report function
https://github.com/qiboteam/qibocal/blob/77f7bd1364a19bb3b749460d63b337e4c6a9e2a1/src/qibocal/cli/report.py#L65
(that already has a pretty simple API), even dropping the executor argument, i.e. assuming that an executor already ran.
What we will need instead will be a History, but I would make that optional (if possible), and try to deserialize the History from the path itself (the purpose of having a History anyhow is to make the input path optional, i.e. you could generate everything in memory, before dumping to disk).
The only other executor attribute used is:
https://github.com/qiboteam/qibocal/blob/77f7bd1364a19bb3b749460d63b337e4c6a9e2a1/src/qibocal/cli/report.py#L99
but we can inherit that in the history.
This should be sufficient for qq report (it is not qq auto, so it's not supposed to run a calibration on its own), and report(path) or report(path, executor.history) should be a simple enough API even for an end user (at which point I'd take out the report function from the cli subpackage, and expose top-level).
We could even do report(executor=executor), in such a way that the path and history are automatically inferred (or report(executor) and check the type, also using @overload for that).
In any case, I'd do this in a separate PR.
alecandido
commented
4 months ago Since this is working, and #869's target is to provide a simpler syntax to use what's already here, I wonder whether we should aim to merge this now.
I will re-review, but I've already approved it, and this PR is just exposing something that is already more or less there in main (i.e. executing routines explicitly with the Executor, now possible individually).
Since it's at an internal stage, we can avoid documenting it as a public feature (the public interface will come with #869), but the improvement is simple and valid on its own.
alecandido
commented
3 months ago @andrea-pasquale @Edoardo-Pedicillo should we then solve the conflicts and merge this PR?
I know we are going to change it even further, in #870 and #869, but this PR is rather self-contained, despite introducing a temporary solution (in any case, main is not a release, and the other two PRs will come quite soon).
Adds possibility to run one protocol at a time using
Executor. Possible script:@alecandido I think this should be more or less what we discussed :)
Checklist:
mastermainmain