Closed alecandido closed 2 months ago
codecov[bot]
commented
4 months ago Attention: Patch coverage is 98.55072% with 1 line in your changes missing coverage. Please review.
Project coverage is 97.23%. Comparing base (
92e775b) to head (e640670).
alecandido
commented
2 months ago I added the call simplification: now it is possible to use the names of the parameters as keyword arguments.
There are three semi-reserved names: id, mode, and parameters itself.
These can not be used as keyword arguments for parameter names, as they are interpreted as other routine attributes.
However, the parameters argument can still be used to pass a dictionary, that will be updated with the further explicit keyword arguments. In that dictionary, there is no reserved key (but using id, mode, and parameters as names for routines' parameters is discouraged anyhow, as it would be confusing, despite being possible).
alecandido
commented
2 months ago The main point of the PR is implemented, and vaguely tested.
However, I'm asking for review to have an actual feedback, not necessarily to merge immediately (and in any case, this is pointing #909, which is pointing #921). Moreover, the script support is not completed, since, to fully reproduce runcards, we need other basic functionalities like those described in #922.
But with a bit of boilerplate scripts could be already used, reimplementing in each and every script the saving operations in https://github.com/qiboteam/qibocal/blob/manage-output/src/qibocal/cli/autocalibration.py and the other commands. So, I'm inclined to merge this with this feature alone, and introduce further features in further PRs, to avoid super-huge PRs and consequently more complicated reviews.
alecandido
commented
2 months ago In b03665e I added a new (small) feature: positional arguments. Now, it should be also possible to omit the name of the parameters, and rely on the order they are listed in the Parameters class.
I checked with the RX calibration script that nothing is broken.
I'm not dedicating a huge effort to properly test all these features, also because they are experimental. As soon as we will improve a bit these scripts (and start using them), I will also expand the testing infrastructure, to make them even more reliable.
alecandido
commented
2 months ago Sorry, I didn't see the review. I'll reply to your comments asap
alecandido
commented
2 months ago Nothing new wrt @andrea-pasquale review, I just rebased on top of the current main.
Here (there will be) the implementation of the rant syntax of #865.
It seems that it is actually possible, and it should result in simple and nice scripts.
Plugins (i.e. protocols set extensions) will also be supported, though we now have the option of avoiding auto-discovery, since there will be room for explicit registration (that seems more intuitive and transparent).
Further proposals
This PR
Actionto be passed as input, we should spread its arguments over its*argsand**kwargs, and later collect them into theAction(providing suitable defaults, whenever possible)example usingcf. #916rxpyprocess outputBeyond
tests are creating files that are left in the repo: we should use thetmp_pathfixture to avoid this (possibly changing working directory there)in particular, the folders:cls_results,data,qubit0..qubit4DataandResultscontain as attributesdict[QubitId, ...]- it would be nicer if this redundancy (the qubit dependence of these values) were managed at a higher level (not individually per-routine)