qiboteam / qibo

A framework for quantum computing
https://qibo.science
Apache License 2.0
287 stars 58 forks source link

Fix complex samples #1414

Closed stavros11 closed 1 month ago

stavros11 commented 1 month ago

This should fix #1412, however I am not sure if it causes any other issues as I don't understand why this cast was introduced and the workflows are failing for other reasons.

Checklist:

scarrazza commented 1 month ago

@renatomello do you remember why the cast was introduced? Due to torch?

alecandido commented 1 month ago

While I agree with the fix, it seems a bit of a patch: in principle, NumpyBackend.cast() is just using np.array(ar, dtype=ar.dtype), so I don't see why a cast to complex should happen...

I should try myself to reproduce, but since you already did it: which was the original object being cast?

stavros11 commented 1 month ago

While I agree with the fix, it seems a bit of a patch:

It is a patch, it is attempting to fix the issue but may not necessarily be the best overall solution.

in principle, NumpyBackend.cast() is just using np.array(ar, dtype=ar.dtype), so I don't see why a cast to complex should happen...

I don't think this is correct. If you look at the NumpyBackend.cast: https://github.com/qiboteam/qibo/blob/259380d337c2520b8a8e73a523a3515207ba525b/src/qibo/backends/numpy.py#L59-L66

if a dtype is not passed it is not maintaining the dtype of the array but rather using self.dtype which is complex64 or complex128 depending on the precision set by the user. QibolabBackend is inheriting all this from NumpyBackend.

I should try myself to reproduce, but since you already did it: which was the original object being cast?

I tried to stop with the debugger right before the cast and the object created by qibolab (in this case gate.result.samples()) has dtype=uint32 which seems right. Therefore, I do not believe the issue is caused by qibolab. It is easy to reproduce, it happens also with the dummy platform which works locally, you don't need the cluster or a real QPU.

I guess it only appears with qibolab because this part of the code is normally not used when simulating. It handles the case where the samples are registered externally, in qibolab's case using what was returned from the instruments. For simulation the samples are generated by the backend (using RNG) so self.measurements[0].result.has_samples() is usually False and thus we fall in the other case of the if statement. However, I am guessing that there are cases even in simulation triggering this line, which is probably why the cast was introduced in the first place, but let's wait for @renatomello to tell us the exact reason.

alecandido commented 1 month ago

if a dtype is not passed it is not maintaining the dtype of the array but rather using self.dtype which is complex64 or complex128 depending on the precision set by the user. QibolabBackend is inheriting all this from NumpyBackend.

You're right, I just got confused with self, and I naively considered it to be the array. But that's true, self in this context is the backend itself.

At this point, I really wonder why it was introduced...

I tried to stop with the debugger right before the cast and the object created by qibolab (in this case gate.result.samples()) has dtype=uint32 which seems right. Therefore, I do not believe the issue is caused by qibolab. It is easy to reproduce, it happens also with the dummy platform which works locally, you don't need the cluster or a real QPU.

Yes, this was just because of my former confusion. But you should be able to reproduce this even with the NumpyBackend itself, just patching the result to contain the samples. We may introduce a test working in this way.

However, I am guessing that there are cases even in simulation triggering this line, which is probably why the cast was introduced in the first place, but let's wait for @renatomello to tell us the exact reason.

Yes, if there is a more likely way to incur in this, we should use that for testing as well.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.94%. Comparing base (259380d) to head (c32fe97).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1414 +/- ## ======================================= Coverage 99.94% 99.94% ======================================= Files 78 78 Lines 11225 11225 ======================================= Hits 11219 11219 Misses 6 6 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibo/pull/1414/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibo/pull/1414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `99.94% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

renatomello commented 1 month ago

@renatomello do you remember why the cast was introduced? Due to torch?

Yes, it is due to torch. Without the cast, the MeasurementOutcumes.samples breaks for torch because it returns a list of np.ndarrays and that breaks further torch functions. torch needs it to be a list of torch.Tensors. I do believe that @stavros11's patch is the easiest, most painless way to solve this.

alecandido commented 1 month ago

Btw, as soon as we'll release v0.2.11 with this patch on PyPI, we should consider yanking v0.2.10, since it's broken for Qibolab and should be avoided (if there is no critical feature in there, we could even yank immediately - but since there are more Qibo users than Qibolab users, maybe it's worth to wait for the new one) @scarrazza