Add operational modes #406
Conversation
|
Since there are some conflicts and I'm modifying the base branch of this PR I would suggest you to wait to review this PR @stavros11 @alecandido. EDIT: too late ahahah |
Co-authored-by: Alessandro Candido <[email protected]>
|
@andrea-pasquale now that you merged #403, this should be merged/rebased on current |
Done @alecandido. I know how to solve it by removing the |
Co-authored-by: Alessandro Candido <[email protected]>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
If you want to merge, just do it.
Everything else could be solved in separate issues.
However, I believe that the current priority of Qibocal is to reduce and simplify the infrastructure. We should try to what we are doing with the minimal (as reasonable amount of) code.
In particular, I would not care too much about what is inside protocols/, but attempt to polish everything else (eventually, I'd also like to review protocols, and improve the code in there, but in a sense we could start considering protocols as plugins, and actually expose a Plugin API for extending Qibocal).
| @dataclass | ||
| class Completed: | ||
| """A completed task.""" | ||
| def add_timings_to_meta(meta, history): |
There was a problem hiding this comment.
The merge function can be arbitrarily improved, e.g.
- we can make a deep merge, recursively
- we can decide which one overwrites the other in case of duplicates
However, if these two are the conditions, or something similar, we could make a simpler and reusable function, rather than spelling out the condition for each field.
The principle is to try to adopt a broader perspective (for simplicity, thus maintainability, and reusability of solutions), instead of solving problems case by case.
Anyhow, it's definite not the uttermost relevant piece of code. So, if merging is more relevant, keep going.
| # FIXME: I'm not too sure why but the code doesn't work anymore | ||
| # with this line. We can postpone it when we will have the | ||
| # ExceptionalFlow. | ||
| # completed.task.iteration += 1 |
There was a problem hiding this comment.
I agree on the conclusion that we should take a deeper look in a separate PR.
However, the idea should be relatively simple: tasks are nodes of the original graph, unique in there, and they all start with zero iterations each.
Since you can pass more than once from each node, multiple completions are possible. Every time you complete a node, you create a completion (Completed instance) with the task id and the current iterations, while you increase the iteration number.
Of course, the task has no path on its own, since data are associated to completions. A completion instead has a path, and it's unique (because tasks are unique in the graph, and each completion of a given task has a unique incremental) and associated to uid = (id, iteration).
If all the assumptions are satisfied, there should be no problem. My guess is that at the moment maximum one iteration is happening, because loops are absent, and the Task is abused in place of the Completed. But I haven't inspected the code.
| @dataclass | ||
| class Completed: | ||
| """A completed task.""" | ||
| def add_timings_to_meta(meta, history): |
There was a problem hiding this comment.
Even better: Completed contains a whole Task, and most likely it is not deeply copied, so it keeps being mutated under the hood.
The easiest is simplifying Completed storing only the task id, such that it could still get access to the task if strictly needed, but with less moving parts.
| ) | ||
|
|
||
|
|
||
| def autocalibrate(card, folder, force, update): |
There was a problem hiding this comment.
For the time being, it is fine like this, at least the classes have been simplified.
However, autocalibrate should be the concatenation of the other three actions (composing them gives us fewer cases), so the function should be call the others.
There was a problem hiding this comment.
I know. Potentially the autocalibrate will become more complicated than a simple acquisition followed by a fitting. But for sure we could recover some parts from acquire and fit. Let's do it in a separate PR as you suggested.
| runcard = Runcard.load(yaml.safe_load((path / RUNCARD).read_text())) | ||
| executor = Executor.load( | ||
| runcard, path, update=update, platform=runcard.platform_obj | ||
| ) |
There was a problem hiding this comment.
These operations are also repeated, so we might consider splitting them.
Not needed now, but keep in mind to reexamine later on.
src/qibocal/protocols/characterization/flux_dependence/utils.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Edoardo Pedicillo <[email protected]>
In this PR I propose to add different operational modes to split acquisition, fitting and report generation.
To achieve this I've added 3 new commands in qibocal:
qq acquire <runcard>which performs the data acquisition (without generating the report)qq fit <output_folder>which performs the fitting/post-processing on a folder containing only the data acquisitionqq report <output_folder>which generates the html report from an output folderqqis now an empty command. To run autocalibration the default command will beqq auto.In this PR I still need to fix tests and coverage.
Things to be fixed before merging:
qq fit(currently it is not possible because the task.run usesyieldand the first output will always be theacquisition)Let me know what you think.
Checklist: