Fix complex samples

[stavros11]

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:

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

[scarrazza]

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

[alecandido]

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]

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:

qibo/src/qibo/backends/numpy.py

Lines 59 to 66 in 259380d

	def cast(self, x, dtype=None, copy=False):
	if dtype is None:
	dtype = self.dtype
	if isinstance(x, self.tensor_types):
	return x.astype(dtype, copy=copy)
	elif self.is_sparse(x):
	return x.astype(dtype, copy=copy)
	return np.array(x, dtype=dtype, copy=copy)

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]

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1414   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          78       78           
  Lines       11225    11225           
=======================================
  Hits        11219    11219           
  Misses          6        6           

Flag	Coverage Δ
unittests	99.94% <ø> (ø)

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.

[renatomello]

@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]

Ok, I'm in favor of the patch. Maybe we should add the test I was proposing above, but given the urgency, we may do it even in a different PR.

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.

Ok, but this problem would be better solved at the level of MeasurementOutcomes itself, which should return a tensor of the given backend (which is then cast to NumPy explicitly, when needed).

I would open an issue to discuss it.

[alecandido]

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