Add optional local qubits parameter

[andrea-pasquale]

Closes #301 and this point

• qubits argument (see first presentation of new layout)

from #336.

This PR adds a local qubits parameter for each routine.
If this parameter is not provided it will be used the global parameter qubits, if provided it will overwrite the global parameter.
Probably this is not enough for supporting two qubit gates routines.
We could address this in another PR once we port two qubit gates to the new layout.

Checklist:

• Reviewers confirm new code works as expected.
• Tests are passing.
• Coverage does not decrease.
• Documentation is updated.

[codecov]

Codecov Report

Merging #351 (fecf5dc) into main (63484ee) will increase coverage by 40.21%.
The diff coverage is 99.35%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #351       +/-   ##
===========================================
+ Coverage   55.22%   95.44%   +40.21%     
===========================================
  Files          76       46       -30     
  Lines        5718     2942     -2776     
===========================================
- Hits         3158     2808      -350     
+ Misses       2560      134     -2426     

Flag	Coverage Δ
unittests	95.44% <99.35%> (+40.21%)	⬆️

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

Impacted Files	Coverage Δ
.../qibocal/protocols/characterization/allxy/allxy.py	100.00% <ø> (ø)
.../characterization/allxy/allxy_drag_pulse_tuning.py	100.00% <ø> (ø)
...c/qibocal/protocols/characterization/rabi/utils.py	100.00% <ø> (+77.77%)	⬆️
...characterization/randomized_benchmarking/params.py	100.00% <ø> (ø)
src/qibocal/protocols/characterization/utils.py	89.36% <ø> (-0.12%)	⬇️
...tocols/characterization/allxy/drag_pulse_tuning.py	97.02% <87.50%> (+0.02%)	⬆️
src/qibocal/__init__.py	100.00% <100.00%> (ø)
src/qibocal/auto/execute.py	98.64% <100.00%> (+0.03%)	⬆️
src/qibocal/auto/runcard.py	97.77% <100.00%> (+0.80%)	⬆️
src/qibocal/auto/task.py	97.64% <100.00%> (+0.54%)	⬆️
... and 18 more

... and 5 files with indirect coverage changes

[alecandido]

@andrea-pasquale

I've re-merged this branch to #384 and it is crashing again:

Thanks @maxhant to report this.

We definitely need qubits in the autocalibration tests, not only to remove Optional[*], but actually to check that the loading is properly working.

[*]: that of course I like

[andrea-pasquale]

Thanks @alecandido and @maxhant.
Regarding the issue about qubit pairs I will fix it in a new PR on top of this one.

[rodolfocarobene]

Thanks @alecandido and @maxhant. Regarding the issue about qubit pairs I will fix it in a new PR on top of this one.

Since there is still no qubit pairs PR, I'm writing here. Just note that also the tests will need some changes:

qibocal/tests/test_operations.py

Line 19 in 61a81a1

task.operation.acquisition(task.parameters, platform, platform.qubits)

[alecandido]

For me, this PR is only missing qubits: key tests
#351 (comment)

[andrea-pasquale]

For me, this PR is only missing qubits: key tests
#351 (comment)

Ok, I can do that.

[alecandido]

I guess you wanted:

Suggested change

	update: bool = None
	update: bool = False

[andrea-pasquale]

No actually None is correct, since this is the local update relative to a protocol, which can be absent.

[alecandido]

But then we are back to the wrong type hint: it is not a bool, that can only be True or False, not None.

If update:

• is a toggle, keep it a bool, and if it can be absent, just choose what is the default value (out of the two allowed)
• if it's an object, then it is not a bool, and if it can be absent, it should be Optional[MyUpdate], and you can set to None

But None can't be a bool. And definitely, do not use an Optional[bool], because this is just 3-states logic (and I believe you will end up acting as if it were one of the other two values).

[andrea-pasquale]

I feel like we are over complicating things. It is an optional value that the user can provide.
I cannot put a default value (True/False) since by providing a value to a local parameter, the local parameter will overwrite the global option which is something that I don't want. If no local option is provided the program should choose the global option.

And definitely, do not use an Optional[bool], because this is just 3-states logic (and I believe you will end up acting as if it were one of the other two values).

I feel like in this case this is exactly what I want. Since my scenario should be the following:

1. if local is provided it will overwrite the global option
2. if local is not provided (local == None) the program should use the global option

But None can't be a bool.

I agree with this.

[alecandido]

You're creating more cases than needed.

I understand the logic of locally prevailing definition (i.e. scopes), but here I believe it could be much simpler.

• by default, you run the autocalibration to update the runcard, so you can make the assumptions you're always updating
• if you want to disable this option for some routines, you can set the local update key to False, for each of them that you want to set to False
• the only reason to set the global one to False is to never update

I believe that you had in mind something like "if I have more False, I set the global to False and switch the few different ones explicitly, otherwise I do the opposite". And that's why you'd like to set an overwritable default.
But this makes all the logic more complicate, because you're allowing every time for two complementary ways of setting the same configuration (global False and set only local Trues, or the opposite).

Instead, only the local key is required to have the full flexibility, so in principle you could avoid having the global one at all.
But for the special (and arguably relevant [*]) case of disabling update for all, you can provide the global key.

[*]: I actually don't know how much this is going to be used; if no one is using it, we can really avoid defining the global key

[andrea-pasquale]

To me this is fine. Since we were discussing about this in #366 with @vodovozovaliza and @wilkensJ. I want to make sure that they also agree.

[andrea-pasquale]

Is this ready to merge @alecandido?

[alecandido]

There would still be bits to be cleaned up. But part of them are already addressed by #387.

I'm becoming mad with all the changes here and in the other PR. If it works, better to merge both, and at most keep optimizing in a further one (after merging both of them)