Replace results with bare arrays

[alecandido]

Resolves #899 and #809

• test new features
• fix tests

[alecandido]

@stavros11 I still have to fix the tests for dummy, but it won't take much.

Instead, the feature should be implemented as it is, and the source code should be stable (unless new bugs will be discovered).
If you want to start giving feedback on that, it'd be welcome.

Initially, I was aiming to address #809 in this PR as well. However, I now realized it is a pretty separate topic (and partially addressed by #885), so I will keep it out of here.

[stavros11]

Thanks @alecandido. I will have another look tomorrow, but I agree for the most part.

Maybe, I am only not fully convinced we need result.py as I am not sure to what extend it is being used in qibolab. If it is just a helper for qibocal, maybe it would make more sense to have it there to avoid breaking interfaces (and version compatibilities) in the future? The probability function does not look very useful anyway (it is quite trivial). The IQ manipulation, on the other hand, (which is pretty clean) could be more useful, but I am not sure if that is in qibolab's context.

For qibolab we could just guarantee that we return the proper shape and type and let the user manipulate it per their needs.

Initially, I was aiming to address #809 in this PR as well. However, I now realized it is a pretty separate topic (and partially addressed by #885), so I will keep it out of here.

Looking at the dummy, I see you dropped qubits from the result keys. Isn't that sufficient for #809 or do we need something more? (assuming all drivers follow the same approach)

[alecandido]

Maybe, I am only not fully convinced we need result.py as I am not sure to what extend it is being used in qibolab. If it is just a helper for qibocal, maybe it would make more sense to have it there to avoid breaking interfaces (and version compatibilities) in the future? The probability function does not look very useful anyway (it is quite trivial). The IQ manipulation, on the other hand, (which is pretty clean) could be more useful, but I am not sure if that is in qibolab's context.

For qibolab we could just guarantee that we return the proper shape and type and let the user manipulate it per their needs.

Yes, in principle, Qibolab could just return the raw arrays as they are. That's the main goal of Qibolab (execute sequences/programs and return the results).

This is still some kind of legacy we're carrying from the time when the Qibolab-Qibocal split was more ambiguous.

In this PR, I'd like to restore previous functionalities, in the absence of the classes above. Thus, I reproduced the small amount of operations that were done by them.
However, right after this we could move the whole of it to Qibocal. And being a very self-contained file, it should be pretty simple (but maybe better to do it when we will start updating consistently also Qibocal, in such a way that we can really move the file from one side to the other).

[alecandido]

Looking at the dummy, I see you dropped qubits from the result keys. Isn't that sufficient for #809 or do we need something more? (assuming all drivers follow the same approach)

Well, I've done nothing explicit in that sense in this PR. What was done happened in #885.

It could be enough (though it's still everywhere in the drivers), but I'd like to make sure it's consistent.
If there will be nothing to do, I'll just close it later on. Otherwise, I'll open a PR to close.

[alecandido]

@stavros11 while improving the tests for the results' shapes, I made such that they are running by default in the CI (with dummy, of course - at some point we could extend to the emulator).
In this way, we make sure that at least the tests and dummy are coordinated, such that there is a unique source of truth.

In the process, I wanted to review the tests for dummy.
While inspecting them, I found out that they are mostly testing the shape as well, but at this point they might have become a bit trivial. I wonder whether we should consider just dropping most of them.

[codecov]

Codecov Report

Attention: Patch coverage is 72.46377% with 19 lines in your changes missing coverage. Please review.

Project coverage is 41.99%. Comparing base (0eaf775) to head (1e214df).
Report is 1 commits behind head on 0.2.

Files	Patch %	Lines
src/qibolab/result.py	76.00%	6 Missing ⚠️
...rc/qibolab/instruments/emulator/pulse_simulator.py	33.33%	4 Missing ⚠️
src/qibolab/instruments/rfsoc/driver.py	20.00%	4 Missing ⚠️
src/qibolab/instruments/icarusqfpga.py	0.00%	3 Missing ⚠️
src/qibolab/instruments/qm/acquisition.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              0.2     #940      +/-   ##
==========================================
- Coverage   42.42%   41.99%   -0.44%     
==========================================
  Files          74       74              
  Lines        5904     5818      -86     
==========================================
- Hits         2505     2443      -62     
+ Misses       3399     3375      -24     

Flag	Coverage Δ
unittests	41.99% <72.46%> (-0.44%)	⬇️

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]

Thanks @alecandido, looks good to me.

In this PR, I'd like to restore previous functionalities, in the absence of the classes above. Thus, I reproduced the small amount of operations that were done by them. However, right after this we could move the whole of it to Qibocal. And being a very self-contained file, it should be pretty simple (but maybe better to do it when we will start updating consistently also Qibocal, in such a way that we can really move the file from one side to the other).

Sounds good to me. Only suggestion would be that if you are planning to remove (move to qibocal) this file before the 0.2 release, maybe we should open an issue and put to the milestone.

@stavros11 while improving the tests for the results' shapes, I made such that they are running by default in the CI (with dummy, of course - at some point we could extend to the emulator). In this way, we make sure that at least the tests and dummy are coordinated, such that there is a unique source of truth.

That's fine for now, however I think we should still have a mechanism to run these tests on instruments, which are more relevant than the dummy for actual applications. I am not sure if the qpu marker is the best option for that, maybe not since it has not been used much in the past. On the other hand, I am not sure if it is possible to test the shape of the output without having access to the instrument. This is certainly for another PR, since we don't have any instruments working here.

In the process, I wanted to review the tests for dummy. While inspecting them, I found out that they are mostly testing the shape as well, but at this point they might have become a bit trivial. I wonder whether we should consider just dropping most of them.

This makes sense since the output of dummy is random in terms of values, so we can only assert the shape. If tests end up being duplicates I agree with dropping. It is also better to run the same test with the dummy and other instruments, instead of having specialized tests only for the dummy. Dummy was added primarily for testing, therefore it may even be an overkill to have tests that test only this.

[stavros11]

Why don't we make this "public"

Suggested change

	class _PulseLike(Model):
	class PulseLike(Model):

to replace the type defined some lines below in

qibolab/src/qibolab/pulses/pulse.py

Line 131 in 1e214df

PulseLike = Union[Pulse, Delay, VirtualZ]

[alecandido]

I tried. It seemed a clever idea, and I did it immediately. But Pydantic doesn't like it during deserialization.

It makes sense: a base class does not know which are its child classes. So, if you only specify that, you don't know in practice what to instantiate.
The parent class is a lower bound ("my class is a child of this"), while to deserialize you need at least an upper bound ("my class is one of these").

However, we can decide about possible renaming, and what to make public. For the time being, I decided not to introduce any other feature: the union was already there and exposed, and this has the only net effect of extending the .id support to the delay and virtual Z.

[alecandido]

Sounds good to me. Only suggestion would be that if you are planning to remove (move to qibocal) this file before the 0.2 release, maybe we should open an issue and put to the milestone.

Done, it's #953

That's fine for now, however I think we should still have a mechanism to run these tests on instruments, which are more relevant than the dummy for actual applications. I am not sure if the qpu marker is the best option for that, maybe not since it has not been used much in the past. On the other hand, I am not sure if it is possible to test the shape of the output without having access to the instrument. This is certainly for another PR, since we don't have any instruments working here.

I did remove the qpu marker, which was excluding these tests in the CI, but it's still possible to run them on hardware (before you should have used the --platform flag, but I found out that the flag was already used by Pytest machinery internally, so now it is called --device).

This makes sense since the output of dummy is random in terms of values, so we can only assert the shape. If tests end up being duplicates I agree with dropping. It is also better to run the same test with the dummy and other instruments, instead of having specialized tests only for the dummy. Dummy was added primarily for testing, therefore it may even be an overkill to have tests that test only this.

Ok, then please have a look yourself, and check whether there is something that is not covered by test_result_shape, and let me know.
Then, we'll decide what to keep, transform, and drop.

[alecandido]

I'm now merging this. For dummy tests, let's open a new PR.

[stavros11]

I'm now merging this. For dummy tests, let's open a new PR.

I opened an issue #954 and for now moved it to later milestone, since it is not interface related. I can still have a look after I am done with more urgent tasks.

[alecandido]

I opened an issue #954 and for now moved it to later milestone, since it is not interface related. I can still have a look after I am done with more urgent tasks.

Yes: I was tempted to list directly for 0.2.0, but 0.2.2 is definitely a better target.
Then we can do even before, but there is no need. I realized recently that the milestones are more useful as a list of commitments, rather than for reporting (for which there is our friend "release drafter" working hard).