Qblox recursion fix #831
Qblox recursion fix #831
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #831 +/- ##
==========================================
+ Coverage 65.09% 66.14% +1.04%
==========================================
Files 50 50
Lines 6034 6035 +1
==========================================
+ Hits 3928 3992 +64
+ Misses 2106 2043 -63
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| def _add_to_results(sequence, results, results_to_add): | ||
| for pulse in sequence.ro_pulses: | ||
| if results[pulse.id]: | ||
| results[pulse.id] += results_to_add[pulse.serial] | ||
| else: | ||
| results[pulse.id] = results_to_add[pulse.serial] | ||
| results[pulse.qubit] = results[pulse.id] |
There was a problem hiding this comment.
This I just copied without changes from existing code, so that I do not duplicate it for the bugfix scenario. However, I am pretty sure this code does not work correctly when there are more than one sweepers. This is not related to the bug this PR is addressing, so once I test and confirm my hypothesis will create another issue for this.
| results=results, | ||
| ) | ||
| @staticmethod | ||
| def _combine_result_chunks(result_chunks): |
There was a problem hiding this comment.
I am thinking of implementing this functionality as some sort of append method for data types IntegratedResults and SampleResults, instead of doing the gymnastics here. The existing methods of those classes make them look like they were designed to be immutable. Is this impression true? If no, I guess just a simple append method that changes each object in place is enough. If yes, I can implement classmethods that accept multiple instances and create a new instance containing the appended (across a certain axis) data. Any comments?
There was a problem hiding this comment.
The existing methods of those classes make them look like they were designed to be immutable. Is this impression true?
It seems that these objects contain numpy arrays which are mutable, so I am not sure if it is possible to make them immutable. Since they are part of the outer interface, it could be good to make them immutable, so when the user executes something they get back an object that doesn't change, however personally I don't have strong opinion on that. If it is not possible due to the array anyway, I do not see any issue with your proposal.
There was a problem hiding this comment.
yeah, they are for sure not immutable objects. I was more thinking along the lines "they are not immutable, but it seems they are intended to be treated as immutable, or used in a manner that pretends as if they are immutable". I think it would be nicer to make them frozen dataclasses. I would do it as another PR. I am not sure if any party currently is relying on the fact that they are mutable. For now I can just go with the classmethod/factory method approach, so that the classes remain immutable-friendly.
There was a problem hiding this comment.
To the best of my understanding, they already have an append-like function, i.e.
Lines 20 to 21 in d080141
and, consequently,
__iadd__ is automatically defined as:
self = self + otherSo, if you want to append, you could use the += operator.
There was a problem hiding this comment.
As @stavros11 said, it could be a good idea to make them immutable (despite I would avoid making the wrestling required to mimic immutability with NumPy array, which are intrinsically mutable, and just pretend them to be immutable, avoiding mutations in Qibolab).
In any case, if you'd like to implement such a feature, please target 0.2. It should definitely be grouped together with the other breaking (and potentially-breaking) changes.
There was a problem hiding this comment.
Btw, my original proposal was to avoid making classes for these results altogether, since they are just the downloaded arrays.
If they are composite, they should have been composite dtypes (see rec arrays).
We could have labels, but they would be essentially the execution parameters. And if we could stick to bare arrays, we could also avoid:
qibolab/src/qibolab/execution_parameters.py
Lines 40 to 51 in d080141
In any case, they are array wrappers. I'd try to keep them as simple as possible...
There was a problem hiding this comment.
So, if you want to append, you could use the += operator.
This is not possible, because I also need to specify the axis along which to append the results. The current implementation of __add__ just appends without specifying any axis, which results into flattening of the arrays. This is why I think the code copied to _add_to_results function will perform incorrectly in certain situations. For now, all I need is to append along axis 0, so I could technically reimplement __add__ to always add things along zeroth axis. However I am not sure if that's easily doable, since I don't know all parties that rely on the current behavior of __add__.
Btw, my original proposal was to avoid making classes for these results altogether, since they are just the downloaded arrays.
This makes sense to me. The current classes have very rudimentary functionality, and that does not seem enough justification for their existence. Plus making them a class hierarchy makes things even more complicated unnecessarily.
There was a problem hiding this comment.
Apart for the combination gymnastics, that I'm not sure I fully understand, the real fix seems avoiding the infinite recursion, by making the final call to actual execution instead.
I'm not sure how this could have ever worked, but the fix was definitely needed.
Anything else seems a consequence, and if @biankawolo could confirm that is working on hardware, I'd be happy like this.
Fixes #830
When the number of bins (
nshots * total_number_of_sweep_points) is larger than the maximum supported by the instrument (2**17), the execution is done in batches. Previously the_sweep_recursionwas called which resulted into infinite recursion stack.Checklist: