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

Simplified calibration scripts #869

Merged
merged 32 commits into from
Jul 17, 2024
Merged

Simplified calibration scripts #869

merged 32 commits into from
Jul 17, 2024

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented May 24, 2024

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

  • the routines call is not optimal for the new simplified layout
    • instead of requesting the full Action to be passed as input, we should spread its arguments over its *args and **kwargs, and later collect them into the Action (providing suitable defaults, whenever possible)
  • example using rxpy cf. Async execution #916
  • process output
    • for the time being, the full output is being returned (it may change in the near future)

Beyond

  • tests are creating files that are left in the repo: we should use the tmp_path fixture to avoid this (possibly changing working directory there)
  • many Data and Results contain as attributes dict[QubitId, ...] - it would be nicer if this redundancy (the qubit dependence of these values) were managed at a higher level (not individually per-routine)
    • if we need to mark this per-routine or per-attribute, we could consider using the fields' metadata for the purpose

@alecandido alecandido changed the title test: Start preparing calibration scripts testing infrastructure Simplified calibration scripts May 24, 2024
Copy link

codecov bot commented May 25, 2024

Codecov Report

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).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #869   +/-   ##
=======================================
  Coverage   97.23%   97.23%           
=======================================
  Files         118      118           
  Lines        8963     9014   +51     
=======================================
+ Hits         8715     8765   +50     
- Misses        248      249    +1     
Flag Coverage Δ
unittests 97.23% <98.55%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/qibocal/__init__.py 100.00% <100.00%> (ø)
src/qibocal/auto/output.py 100.00% <100.00%> (ø)
src/qibocal/auto/task.py 98.05% <100.00%> (+0.01%) ⬆️
src/qibocal/cli/acquisition.py 95.83% <100.00%> (+0.18%) ⬆️
src/qibocal/cli/autocalibration.py 96.15% <100.00%> (+0.15%) ⬆️
src/qibocal/cli/report.py 100.00% <ø> (ø)
src/qibocal/web/report.py 100.00% <100.00%> (ø)
src/qibocal/auto/execute.py 98.79% <98.18%> (-1.21%) ⬇️

Base automatically changed from run_single_protocol to main June 6, 2024 05:49
@alecandido alecandido mentioned this pull request Jun 12, 2024
This was referenced Jun 27, 2024
@alecandido alecandido changed the base branch from main to manage-output July 5, 2024 16:26
@alecandido
Copy link
Member Author

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).

def wrapper(parameters=None, id=name, mode=AUTOCALIBRATION, **kwargs):
params = parameters.copy() if parameters is not None else {}
params = {"id": id, "parameters": params | kwargs}
return executor.run_protocol(protocol, parameters=params, mode=mode)

@alecandido alecandido marked this pull request as ready for review July 5, 2024 18:03
@alecandido
Copy link
Member Author

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
Copy link
Member Author

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.

src/qibocal/auto/execute.py Outdated Show resolved Hide resolved
src/qibocal/auto/execute.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member Author

Nothing new wrt @andrea-pasquale review, I just rebased on top of the current main.

Copy link
Contributor

@Edoardo-Pedicillo Edoardo-Pedicillo left a comment

Choose a reason for hiding this comment

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

Thanks @alecandido, LGTM

@alecandido alecandido added this pull request to the merge queue Jul 17, 2024
Merged via the queue into main with commit 46df4ef Jul 17, 2024
21 checks passed
@alecandido alecandido deleted the import-from-executor branch July 17, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants