Fix crash when there is a gap in input data - #1077
Conversation
At present the per-spectrum values are just added up on the host to give a per-chunk value as before, but this paves the way to fixing NGC-2074.
The GPU now accumulates data up to batch granularity rather than just spectrum granularity (which was too expensive). This still doesn't have any benefit as is, but is on the path to fixing NGC-2074.
Instead of testing every iteration whether it is time to emit accumulated results, use nested loops with the emission in the outer loop.
Now that power stats are collected per-batch instead of per-chunk, we no longer require the window size to be a multiple of the chunk size (just of the batch size).
This brings it into line with the other arrays, which are polarisation-major. It also removes the need to transpose it later on the CPU.
|
I'm requesting @vincentseotlo as primary reviewer. @EatonEmmerich is added more for interest / learning. |
| int offset = group_y * stepy + pos; // This thread probably doesn't work on the very beginning of the data, so we | ||
| // make the indexing easier for ourselves later. | ||
| // pos is the position within the step (i.e. spectrum) that this thread will work on. | ||
| int pos = get_group_id(0) * WGS + lid; |
There was a problem hiding this comment.
I think we can keep the standard of using group_x, so we're in line with group_y. Granted group_y is shifted by stepy.
| # ensures that tests that it divides into other values are | ||
| # satisfied. | ||
| total_power_spectra = 1 | ||
| self.total_power_spectra = total_power_spectra |
There was a problem hiding this comment.
Is there any danger in just saying self.total_power_spectra = 1, then line 138: "total_power_spectra": self.total_power_spectra,
| else: | ||
| sensor.set_value( | ||
| np.finfo(np.float64).min, status=aiokatcp.Sensor.Status.FAILURE, timestamp=update_timestamp | ||
| ) |
There was a problem hiding this comment.
what do we expect to happen if we accumulate with power=None on an existing value in the accumulator?
| @pytest.fixture | ||
| def dig_rms_dbfs_window_chunks() -> int: | ||
| def dig_rms_dbfs_window_batches() -> int: | ||
| """Number of chunks per window for ``dig-rms-dbfs`` sensors.""" | ||
| return 2 | ||
| return 5 |
There was a problem hiding this comment.
why not just use a constant?
| // Load the raw data for the sample | ||
| sample_t sample = unpack_read(&unpack); | ||
| unpack_advance(&unpack, step); | ||
| { // Block just to balance things with the !complex_input case. |
There was a problem hiding this comment.
nit(non-blocking): missed the ! here first time I read the comment and got confused.
Suggest writing it as not complex_input case.
|
|
||
| if dig_total_power is not None: | ||
| self._update_dig_power_sensors(dig_total_power_windows, dig_total_power, out_item) | ||
| self._update_dig_power_sensors(dig_total_power_windows, dig_total_power, out_item, chunk.present) |
There was a problem hiding this comment.
can we pass the batch_samples in here instead then calculate it from the spectra_per_heap like we do in _dig_rms_dbfs_window_samples?
3 participants
When there is a gap between input chunks, the output chunk is fast-forwarded to avoid referencing missing data. This is done with batch granularity, so as not to lose more output data than necessary. However, that means that the OutQueueItem is not necessarily aligned (in time) to a multiple of the output chunk size. In our unit tests it generally has been, because there is no delay model and so it ends up aligned to the input chunk size; but when there is a non-zero coarse delay, that's no longer the case. Meanwhile, the code for handling the dig-rms-dbfs sensor works with "windows" which are some multiple of the chunk size and aligned to the window size, and does not cope with OutQueueItems that cross a window boundary. In theory this could also be an issue if a non-zero delay was set before the first data arrived.
Fix it by changing the computation of total digitiser power from being done per-output-chunk (on the GPU) to per-output-batch. I initially experimented with doing it per-output-spectrum, but that was too expensive; even with per-output-batch it needed some experimentation to get the CUDA compiler to generate reasonable code that didn't slow things down massively. I've tested with benchmark_fgpu and found no statistically significant impact on performance:
Add a non-zero delay to the existing unit tests for missing data. That made it a lot more complicated unfortunately.
Checklist (if not applicable, edit to add
(N/A)and mark as done):pyproject.tomland.pre-commit-config.yaml.fake_servers.pyin katsdpcontroller to match.VERSION constant for the engine (update the major number if sensors/requests have been
removed/changed, minor number if only backwards-compatible changes have been done).
Also, update doc/control.rst with the new version.
Closes NGC-2074.