Minor fixes in VZ protocol#1261
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1261 +/- ##
=======================================
Coverage 91.94% 91.94%
=======================================
Files 147 147
Lines 11581 11588 +7
=======================================
+ Hits 10648 10655 +7
Misses 933 933
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| if rec_array: | ||
| target_data = data[target, control, setup].target | ||
| else: | ||
| target_data = data[target, control, setup][0] |
There was a problem hiding this comment.
Isn't it possible to standardize the usage, and keep it just one in the end?
I see that before was a record array. And, since it has proved being inconvenient, now Qibocal is more or less to regular arrays. Which is certainly reasonable[*].
But, then, why not just transitioning everything to regular arrays? At least in related places, such that we avoid these kinds of forks.
[*]: We may discuss why it has been inconvenient, which are instead the advantages, and whether it is worth to attempt retaining them, changing instead something else. But I assume you already did this assessment, more or less in depth. So, fine, we can certainly go back to regular arrays - if nothing else, just because of simplicity. Which is a good motivation
There was a problem hiding this comment.
I'm planning to do that as well. The SNZ protocols are still using recarrays.
| f"Qubit {qubits[1]}", | ||
| ), | ||
| ) | ||
| print(fit) |
| return True | ||
| # pairs = { | ||
| # (target, control) for target, control, _, _, _ in self.fitted_parameters | ||
| # } | ||
| # return key in pairs |
There was a problem hiding this comment.
Yeah this was used for debugging as well. I will fix it.
550fada to
e92af47
Compare
eb0ffaa to
c3018ae
Compare
|
@scarrazza while we didn't carefully review this, the state of main is also unknown and presumably this is an improvement. Hence it probably makes sense to merge this branch, benefitting from what Andrea did here, rather than to work on top of current main. |
|
@RoyStegeman please go ahead. |
Closes #1260
Here is a report: http://login.qrccluster.com:9000/QWgaFRHzTN6Cc7oVR7I8yw==/