Make Sweeper pydantic Model and introduce range

[stavros11]

As discussed in #540 this drops the SweeperType from the interface.

QM is not updated, but since it never supported SweeperType before, it will still be working same as before. Following the discussion in #540, I need to change all sweepers to behave as ABSOLUTE. I will do this in another PR.

[codecov]

Codecov Report

Attention: Patch coverage is 88.09524% with 5 lines in your changes missing coverage. Please review.

Project coverage is 51.86%. Comparing base (8887534) to head (adbcb38).
Report is 16 commits behind head on 0.2.

Files with missing lines	Patch %	Lines
src/qibolab/instruments/zhinst/sweep.py	0.00%	2 Missing ⚠️
src/qibolab/instruments/qm/program/instructions.py	0.00%	1 Missing ⚠️
src/qibolab/instruments/qm/program/sweepers.py	0.00%	1 Missing ⚠️
src/qibolab/instruments/zhinst/executor.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              0.2    #1014      +/-   ##
==========================================
+ Coverage   51.77%   51.86%   +0.08%     
==========================================
  Files          58       58              
  Lines        2760     2765       +5     
==========================================
+ Hits         1429     1434       +5     
  Misses       1331     1331              

Flag	Coverage Δ
unittests	51.86% <88.09%> (+0.08%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stavros11]

@alecandido I changed Sweeper to pydantic Model in 5205f97. This had the side-effect of breaking various things including tests, because it doesn't support positional arguments anymore.

Currently from tests only test_dummy is broken. I can have a look and fix them, but I am not sure if it's worth it given #996. Maybe something in the docs is also broken, so I will convert to draft until I check and also confirm it didn't break QM.

[alecandido]

@alecandido I changed Sweeper to pydantic Model in 5205f97. This had the side-effect of breaking various things including tests, because it doesn't support positional arguments anymore.

The example in the issue was Pydantic-based. But only because I'm personally leaning towards it, it's perfectly fine to use dataclasses in many cases.

The two motivations I see for using Pydantic are:

1. better (de)serialization support - the reason why I introduced it
2. more subclassing-friendly, that is the reason why I extended to Instrument (since there were many subclasses everywhere)

None of the two strict applies to the Sweeper. However, it is not a bad idea, simply not necessary. I expect a minor amount of fixes required anyhow.

Currently from tests only test_dummy is broken. I can have a look and fix them, but I am not sure if it's worth it given #996. Maybe something in the docs is also broken, so I will convert to draft until I check and also confirm it didn't break QM.

Regarding test_dummy we could even merge #996 immediately, it is ready. I was only waiting to drop some more tests, but I can convert the comment #996 (comment) to an issue with the missing points, and apply those in a separate PR.

About the docs, I'd suggest you taking a look at:

qibolab/doc/source/conf.py

Lines 21 to 24 in c180cd9

	# TODO: the following is a workaround for Sphinx doctest, cf.
	# - https://github.com/qiboteam/qibolab/commit/e04a6ab
	# - https://github.com/pydantic/pydantic/discussions/7763
	import qibolab.instruments.zhinst

Pydantic is a perfectly valid Python compiled extension, but Sphinx is going deeper below the surface than what Python strictly guarantees, apparently (indeed, in the past, I remember to have whitelisted even NumPy or other compiled extensions - maybe there were better solutions, but it was relevant that the issue was raised).
Pydantic has the added "benefit" that the BaseModel is made for subclassing. Importing the offending modules in conf.py seems to solve the issue (for unknown reasons related to the inner machinery of Sphinx)

[alecandido]

Suggested change

	linspace: Optional[tuple[float, float, float]] = None
	range: Optional[tuple[float, float, float]] = None

?

@andrea-pasquale, @Edoardo-Pedicillo: any advice for a better name than those two?

[andrea-pasquale]

What are values and linspace in this case?

[stavros11]

Two different ways to define the sweeper values. values is an array containing all the points, same as in 0.1. range will be a tuple with three elements (start, stop, step) and the values will be defined as np.arange(start, stop, step). Only one of them should be given when initializing the Sweeper. If both are given it will raise an error.

[stavros11]

Changed in 2726f27.

I forgot to say above that the idea is that range should be used whenever possible, as it is expected to be more efficient for most instruments. It should also be sufficient for most qibocal routines. values is still there for special cases where non-equidistant points need to be swept for some reason.

[andrea-pasquale]

Thanks for clarifying, I would say that range is fine.

[stavros11]

This should now also be ready. @alecandido what do you think about populating the actual values from range (when given) in some kind of __post_init__ (I am not sure if it is possible), instead of having the values_array property? The main reason is that I don't like much values_array, neither vals from the original proposal since it's not an actual word. I would like to use values both for initializing the Sweeper but also to return the values if the Sweeper was defined using range.

Note that drivers can still check if a Sweeper is range as they can check if sweeper.range is not None before accessing values.

[alecandido]

This should now also be ready. @alecandido what do you think about populating the actual values from range (when given) in some kind of __post_init__ (I am not sure if it is possible)

Possible, it's called model_post_init. Instead of dunders (i.e. __xxx__) Pydantic is using a model_* scope.
https://docs.pydantic.dev/latest/api/base_model/#pydantic.BaseModel.model_post_init

With Sweeper now being frozen, it can make even more sense: it's true that you're doing some (tiny) potentially unneeded computation ahead of time (with the property, you would compute the array from .range only if needed). But this is negligible.
Instead, the non-negligible risk was getting out-of-sync after instantiation, which is not possible (in a simple way), because it's frozen.

The main reason is that I don't like much values_array, neither vals from the original proposal since it's not an actual word. I would like to use values both for initializing the Sweeper but also to return the values if the Sweeper was defined using range.

We could find a better word. But there is some value in keeping a single way to access, because it would be less confusing.
In order to make proper use of the property, the attribute should have been private. Which it can't be, if we want it to be also a "field".

Note that drivers can still check if a Sweeper is range as they can check if sweeper.range is not None before accessing values.

This should not even happen: if a driver is capable of using (properly) the array, it should directly aim for that. And it will always be available.
If it isn't, it should just consume .range, and fail if not present (since the arbitrary array will be unsupported).

[alecandido]

The model_post_init is only one option, and it's the one to be used when:

This is useful if you want to do some validation that requires the entire model to be initialized.
https://docs.pydantic.dev/latest/api/base_model/#pydantic.BaseModel.model_post_init

And most likely this is our case.

However, even "after" validators can do a very similar job, and, e.g., you can assign Sweeper.values here (if None), right after having checked that the two of them are not provided simultaneously.

[stavros11]

Thanks @alecandido. I made the change and dropped the values_array property in c5929b4.

[stavros11]

Actually, it seems that I cannot change self.values because the object is frozen. I am checking how to fix.

[alecandido]

I made some changes myself, to slightly simplify qibolab.sweeper. Please, check them before merging.

Other than that, this is good to go.

[stavros11]

Thanks @alecandido, looks good to me. I will merge when the CI passes.